Improve: binaryops with number protocol #4139
Conversation
6b4373a to
b8619f1
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
844a5b6 to
ee890ad
Compare
| @@ -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>( | |||
There was a problem hiding this comment.
| pub fn methods<'a>( | |
| pub fn binary_op<'a>( |
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
a54c364 to
b914c7f
Compare
b914c7f to
fc55c89
Compare
|
|
||
| if let Some(slot_a) = slot_a { | ||
| if let Some(slot_b) = slot_b { | ||
| // Check if `a` is subclass of `b` |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| &'a self, | ||
| op_slot: &'a PyNumberMethodsOffset, | ||
| vm: &VirtualMachine, | ||
| ) -> PyResult<&BinaryFunc> { |
There was a problem hiding this comment.
if we keep the name binary_op, the return type will be something other than BinaryFunc
| // 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( |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
How about using ret.fast_isinstance(PyNotImplemented::class(self)) to check self.ctx.not_implemented?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
merged through #4615 |
Environments
rustpythonbinary generated after usingcargo run --releasefor faster runtime than debug one.Tasks
multiplyfor testPyNumberBinaryOpimplementationor=>type_::or_()problemand=>settype doesn't have number protocolmod(remainder)=>strmodule - implant number protocol, compatible with bytes.rs's remainder methodProfiling
Benchmark source
Let me know if you have any good idea for improved benchmark!
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.