1. Apply only formultiply 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.pyimportpyperfimportsysITERATIONS=1000000defmultiply():integer=1floating=1.0byte_str=b'1'foriinrange(ITERATIONS):integer= (integer*i)%sys.maxsizefloating= (floating*float(i))%sys.maxsizebyte_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-stackhas 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    |+-----------+-----------------------+---------------------------+----------------------------+

Karatuss force-pushed theimprove/vm_ops-pynumber branch from6b4373a tob8619f1Compare

August 28, 2022 08:47

youknowone requested a review fromqingshi163

August 28, 2022 16:36

This comment was marked as outdated.

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

Aug 29, 2022

Karatuss force-pushed theimprove/vm_ops-pynumber branch from844a5b6 toee890adCompare

August 29, 2022 08:47

youknowone reviewed

Aug 31, 2022

View reviewed changes

vm/src/vm/vm_ops.rs OutdatedShow resolvedHide resolved

youknowone reviewed

Sep 3, 2022

View reviewed changes

vm/src/builtins/bytes.rsShow resolvedHide resolved

vm/src/protocol/number.rsShow resolvedHide resolved

youknowone reviewed

Sep 3, 2022

View reviewed changes

vm/src/protocol/number.rsShow resolvedHide resolved

youknowone reviewed

Sep 3, 2022

View reviewed changes

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>(

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change

	pubfnmethods<'a>(
	pubfnbinary_op<'a>(

vm/src/protocol/number.rsShow resolvedHide resolved

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.

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

If allowing arbitary n is not necessary for cpython compatibility, I thinkrepeat is better to takeisize orusize because we can skip range check when we already know repeatingn is possible.
If we are going to remove repetitive pattern, addingtry_repeat with more inclusiven will be possible.

Copy link

Member

youknowoneSep 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

youknowone force-pushed theimprove/vm_ops-pynumber branch froma54c364 tob914c7fCompare

February 21, 2023 09:50

youknowone mentioned this pull request

Feb 21, 2023

Fix sequence mul types and add AsNumber for PyUnion#4535

Merged

Karatussand others added2 commits

February 22, 2023 01:06

Binary ops with number protocol

39cd1f2

fix clippy

fc55c89

youknowone force-pushed theimprove/vm_ops-pynumber branch fromb914c7f tofc55c89Compare

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

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`

Copy link

Contributor

xiaozhiyanFeb 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@youknowone

According to CPython implementationhttps://github.com/python/cpython/blob/main/Objects/abstract.c#L842 , we may need to ensureb is a "strict subclass" ofa in order to useslot_b for operation.

fast_isinstance method seems including the situation thata &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?

Copy link

Member

youknowoneFeb 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 reviewed

Feb 28, 2023

View reviewed changes

vm/src/protocol/number.rs

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

Copy link

Member

youknowoneFeb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

if we keep the namebinary_op, the return type will be something other thanBinaryFunc

xiaozhiyan reviewed

Mar 2, 2023

View reviewed changes

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(

Copy link

Contributor

xiaozhiyanMar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@youknowone
Isrich_compare_bool slow since it calls_cmp which is not a trivial function?
Is it the suggested way to check whetherret isself.ctx.not_implemented?

Copy link

Contributor

xiaozhiyanMar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

How about usingret.fast_isinstance(PyNotImplemented::class(self)) to checkself.ctx.not_implemented?

Copy link

Member

youknowoneMar 2, 2023•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

BecauseNotImplemented 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.

Copy link

Member

youknowoneMar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Oh, I missed the second comment. In this case,fast_isinstance doesn't have any advantage to simpleis calling. Becausefast_isinstance doesn't check__isinstance__ python-implemented weird things will not be covered. But becauseNotImplemented is always guaranteed to be singleton,fast_isinstance doesn't check more thing. Sofast_isinstance is not strong asrich_compare but same weak as simpleis.