Improve: binaryops with number protocol #4139

Karatuss · 2022-08-28T07:34:50Z

Environments

Use rustpython binary generated after using cargo run --release for faster runtime than debug one.

Tasks

1. Apply only for multiply for test
2. Apply for every number methods
- Sub-tasks
  - ~~PyNumberBinaryOp implementation~~
  - extra_tests/snippets/builtin_complex.py: fix multiply
  - or => type_::or_() problem
  - and => set type doesn't have number protocol
  - mod(remainder) => str module - implant number protocol, compatible with bytes.rs's remainder method
3. Refactor relative codes being useless, and so on.

Profiling

Benchmark source

Let me know if you have any good idea for improved benchmark!

# benches/microbenchmarks/multiply.py

import pyperf
import sys

ITERATIONS = 1000000

def multiply():
    integer = 1
    floating = 1.0
    byte_str = b'1'

    for i in range(ITERATIONS):
        integer = (integer * i) % sys.maxsize
        floating = (floating * float(i)) % sys.maxsize
        byte_str = byte_str * (i % 1000)
        # a = i % sys.maxsize
        # complexing = complex(a, a) * a

# multiply()
if __name__ == "__main__":
    runner = pyperf.Runner()
    runner.bench_func('multiply', multiply)

flamegraph

Before

After

You can check the call-stack has decreased comparing with before!

pyperf

This represents binaryops with number protocol can improve RustPython's performance just by applying only for multiplying operation.

+-----------+-----------------------+---------------------------+----------------------------+
| Benchmark | multiply_cpython_3_10 | multiply_rustpython_after | multiply_rustpython_before |
+===========+=======================+===========================+============================+
| multiply  | 340 ms                | 5.57 sec: 16.36x slower   | 6.14 sec: 18.05x slower    |
+-----------+-----------------------+---------------------------+----------------------------+

vm/src/vm/vm_ops.rs

vm/src/builtins/bytes.rs

vm/src/protocol/number.rs

youknowone · 2022-09-03T06:04:29Z

vm/src/protocol/number.rs

@@ -237,8 +330,12 @@ impl PyNumber<'_> {
        obj.class().mro_find_map(|x| x.slots.as_number.load())
    }

-    pub fn methods(&self) -> &PyNumberMethods {
-        self.methods
+    pub fn methods<'a>(


Suggested change

pub fn methods<'a>(

pub fn binary_op<'a>(

vm/src/protocol/number.rs

youknowone · 2022-09-03T06:16:58Z

vm/src/vm/vm_ops.rs

+            let seq_a = PySequence::new(a, vm);
+            let seq_b = PySequence::new(b, vm);
+
+            // TODO: I think converting to isize process should be handled in repeat function.


who is "I"? the future reader will be confused.

If you think this topic requires discussion rather than leaving concrete TODO lists,
please consider opening an issue and leave here the issue number or link.

If allowing arbitary n is not necessary for cpython compatibility, I think repeat is better to take isize or usize because we can skip range check when we already know repeating n is possible.
If we are going to remove repetitive pattern, adding try_repeat with more inclusive n will be possible.

another idea is this will be a temporary code.
once all sequences implement number protocol correctly, they will handle it in their own number protocol.

xiaozhiyan · 2023-02-25T21:48:34Z

vm/src/vm/vm_ops.rs

+
+        if let Some(slot_a) = slot_a {
+            if let Some(slot_b) = slot_b {
+                // Check if `a` is subclass of `b`


@youknowone

According to CPython implementation https://github.com/python/cpython/blob/main/Objects/abstract.c#L842 , we may need to ensure b is a "strict subclass" of a in order to use slot_b for operation.

fast_isinstance method seems including the situation that a & b belong to the same class, https://github.com/RustPython/RustPython/blob/main/vm/src/object/ext.rs#L459 .
Is there a method or proper approach to check "strict subclass" please?

I don't think we have a function to do that.
But how about !a.is(b) && a.fast_issubclass(b)?
when a and b are identical type, a.is(b) will be true

youknowone · 2023-02-28T09:05:43Z

vm/src/protocol/number.rs

+        &'a self,
+        op_slot: &'a PyNumberMethodsOffset,
+        vm: &VirtualMachine,
+    ) -> PyResult<&BinaryFunc> {


if we keep the name binary_op, the return type will be something other than BinaryFunc

xiaozhiyan · 2023-03-02T06:39:22Z

vm/src/vm/vm_ops.rs

+                // Check if `a` is subclass of `b`
+                if b.fast_isinstance(a.class()) {
+                    let ret = slot_b(num_a, b, self)?;
+                    if ret.rich_compare_bool(


@youknowone
Is rich_compare_bool slow since it calls _cmp which is not a trivial function?
Is it the suggested way to check whether ret is self.ctx.not_implemented?

How about using ret.fast_isinstance(PyNotImplemented::class(self)) to check self.ctx.not_implemented?

Because NotImplemented is a singleton, I think this check can be fine:

ret.is(&self.ctx.not_implemented)

But not 100% sure because some type can override operators in weird way and rich_compare_bool will respect it. I have no idea about how CPython handle it.

Oh, I missed the second comment. In this case, fast_isinstance doesn't have any advantage to simple is calling. Because fast_isinstance doesn't check __isinstance__ python-implemented weird things will not be covered. But because NotImplemented is always guaranteed to be singleton, fast_isinstance doesn't check more thing. So fast_isinstance is not strong as rich_compare but same weak as simple is.

from RustPython#4139 RustPython#4139

youknowone · 2023-03-11T06:06:00Z

merged through #4615

Karatuss force-pushed the improve/vm_ops-pynumber branch from 6b4373a to b8619f1 Compare August 28, 2022 08:47

youknowone requested a review from qingshi163 August 28, 2022 16:36

This comment was marked as outdated.

Sign in to view

youknowone added the z-ca-2022 Tag to track contrubution-academy 2022 label Aug 29, 2022

Karatuss force-pushed the improve/vm_ops-pynumber branch from 844a5b6 to ee890ad Compare August 29, 2022 08:47

youknowone reviewed Aug 31, 2022

View reviewed changes

vm/src/vm/vm_ops.rs Outdated Show resolved Hide resolved

youknowone reviewed Sep 3, 2022

View reviewed changes

vm/src/builtins/bytes.rs Show resolved Hide resolved

vm/src/protocol/number.rs Show resolved Hide resolved

youknowone reviewed Sep 3, 2022

View reviewed changes

vm/src/protocol/number.rs Show resolved Hide resolved

youknowone reviewed Sep 3, 2022

View reviewed changes

youknowone force-pushed the improve/vm_ops-pynumber branch from a54c364 to b914c7f Compare February 21, 2023 09:50

youknowone mentioned this pull request Feb 21, 2023

Fix sequence mul types and add AsNumber for PyUnion #4535

Merged

Karatuss and others added 2 commits February 22, 2023 01:06

Binary ops with number protocol

39cd1f2

fix clippy

fc55c89

youknowone force-pushed the improve/vm_ops-pynumber branch from b914c7f to fc55c89 Compare February 21, 2023 16:07

youknowone mentioned this pull request Feb 24, 2023

Implement protocols (Abstract obejcts layer) #3244

Open

xiaozhiyan reviewed Feb 25, 2023

View reviewed changes

youknowone reviewed Feb 28, 2023

View reviewed changes

xiaozhiyan reviewed Mar 2, 2023

View reviewed changes

xiaozhiyan mentioned this pull request Mar 2, 2023

Improve: binary ops with Number Protocol #4615

Merged

youknowone pushed a commit to xiaozhiyan/RustPython that referenced this pull request Mar 3, 2023

Binary ops with number protocol (partially imported)

e5c0961

from RustPython#4139 RustPython#4139

xiaozhiyan pushed a commit to xiaozhiyan/RustPython that referenced this pull request Mar 3, 2023

Binary ops with number protocol (partially imported)

2d430fb

from RustPython#4139 RustPython#4139

xiaozhiyan pushed a commit to xiaozhiyan/RustPython that referenced this pull request Mar 4, 2023

Binary ops with number protocol (partially imported)

6fed574

from RustPython#4139 RustPython#4139

xiaozhiyan pushed a commit to xiaozhiyan/RustPython that referenced this pull request Mar 5, 2023

Binary ops with number protocol (partially imported)

8212b64

from RustPython#4139 RustPython#4139

xiaozhiyan pushed a commit to xiaozhiyan/RustPython that referenced this pull request Mar 5, 2023

Binary ops with number protocol (partially imported)

ae98bd4

from RustPython#4139 RustPython#4139

youknowone pushed a commit to youknowone/RustPython that referenced this pull request Mar 5, 2023

Binary ops with number protocol (partially imported)

ef0eae0

from RustPython#4139 RustPython#4139

xiaozhiyan pushed a commit to xiaozhiyan/RustPython that referenced this pull request Mar 6, 2023

Binary ops with number protocol (partially imported)

18b7a02

from RustPython#4139 RustPython#4139

youknowone pushed a commit to youknowone/RustPython that referenced this pull request Mar 6, 2023

Binary ops with number protocol (partially imported)

3db0d19

from RustPython#4139 RustPython#4139

xiaozhiyan pushed a commit to xiaozhiyan/RustPython that referenced this pull request Mar 6, 2023

Binary ops with number protocol (partially imported)

eb41b6a

from RustPython#4139 RustPython#4139

xiaozhiyan pushed a commit to xiaozhiyan/RustPython that referenced this pull request Mar 6, 2023

Binary ops with number protocol (partially imported)

65db767

from RustPython#4139 RustPython#4139

xiaozhiyan pushed a commit to xiaozhiyan/RustPython that referenced this pull request Mar 7, 2023

Binary ops with number protocol (partially imported)

67a434e

from RustPython#4139 RustPython#4139

xiaozhiyan pushed a commit to xiaozhiyan/RustPython that referenced this pull request Mar 7, 2023

Binary ops with number protocol (partially imported)

65ec2d7

from RustPython#4139 RustPython#4139

xiaozhiyan pushed a commit to xiaozhiyan/RustPython that referenced this pull request Mar 7, 2023

Binary ops with number protocol (partially imported)

ef03f31

from RustPython#4139 RustPython#4139

xiaozhiyan pushed a commit to xiaozhiyan/RustPython that referenced this pull request Mar 7, 2023

Binary ops with number protocol (partially imported)

5783807

from RustPython#4139 RustPython#4139

xiaozhiyan pushed a commit to xiaozhiyan/RustPython that referenced this pull request Mar 7, 2023

Binary ops with number protocol (partially imported)

930442d

from RustPython#4139 RustPython#4139

xiaozhiyan pushed a commit to xiaozhiyan/RustPython that referenced this pull request Mar 7, 2023

Binary ops with number protocol (partially imported)

5302e03

from RustPython#4139 RustPython#4139

xiaozhiyan pushed a commit to xiaozhiyan/RustPython that referenced this pull request Mar 9, 2023

Binary ops with number protocol (partially imported)

687156c

from RustPython#4139 RustPython#4139

xiaozhiyan pushed a commit to xiaozhiyan/RustPython that referenced this pull request Mar 9, 2023

Binary ops with number protocol (partially imported)

bffb83e

from RustPython#4139 RustPython#4139

xiaozhiyan pushed a commit to xiaozhiyan/RustPython that referenced this pull request Mar 9, 2023

Binary ops with number protocol (partially imported)

fd9acc9

from RustPython#4139 RustPython#4139

xiaozhiyan pushed a commit to xiaozhiyan/RustPython that referenced this pull request Mar 9, 2023

Binary ops with number protocol (partially imported)

e4c0fa7

from RustPython#4139 RustPython#4139

xiaozhiyan pushed a commit to xiaozhiyan/RustPython that referenced this pull request Mar 9, 2023

Binary ops with number protocol (partially imported)

13d715e

from RustPython#4139 RustPython#4139

minhrongcon2000 pushed a commit to minhrongcon2000/RustPython that referenced this pull request Mar 10, 2023

Binary ops with number protocol (partially imported)

3e102a6

from RustPython#4139 RustPython#4139

youknowone closed this Mar 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve: binaryops with number protocol #4139

Improve: binaryops with number protocol #4139

Karatuss commented Aug 28, 2022 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

youknowone Sep 3, 2022

youknowone Sep 3, 2022

youknowone Sep 3, 2022

youknowone Sep 3, 2022

xiaozhiyan Feb 25, 2023

youknowone Feb 26, 2023

youknowone Feb 28, 2023

xiaozhiyan Mar 2, 2023

xiaozhiyan Mar 2, 2023

youknowone Mar 2, 2023 •

edited

Loading

youknowone Mar 2, 2023

youknowone commented Mar 11, 2023

Improve: binaryops with number protocol #4139

Improve: binaryops with number protocol #4139

Conversation

Karatuss commented Aug 28, 2022 • edited Loading

Environments

Tasks

Profiling

Benchmark source

flamegraph

pyperf

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowone Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowone commented Mar 11, 2023

Karatuss commented Aug 28, 2022 •

edited

Loading

youknowone Mar 2, 2023 •

edited

Loading