NotificationsYou must be signed in to change notification settings
Fork1.3k
Star20.3k

Implement Number Protocol#3507

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

youknowone merged 7 commits intoRustPython:mainfromqingshi163:number-protocol

May 29, 2022

Merged

Implement Number Protocol#3507

youknowone merged 7 commits intoRustPython:mainfromqingshi163:number-protocol

May 29, 2022

Conversation

Copy link

Contributor

qingshi163 commentedDec 20, 2021•
edited by youknowone
Loading

number protocol of#3244

youknowone force-pushed thenumber-protocol branch from243a130 tod5585f0Compare

April 18, 2022 15:15

Copy link

Member

youknowone commentedApr 26, 2022

@qingshi163 what have to be done to finish this PR?

qingshi163 force-pushed thenumber-protocol branch fromd5585f0 to55659c3Compare

May 9, 2022 19:32

qingshi163 force-pushed thenumber-protocol branch from5215967 to40a8890Compare

May 22, 2022 19:00

Copy link

Member

youknowone commentedMay 23, 2022

awesome!

youknowone reviewed

May 25, 2022

View reviewed changes

Copy link

Member

youknowone left a comment•
edited
Loading

Choose a reason for hiding this comment

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

I have questions about cow and OnceCell part, but it looks working!
This is the way to go and we can fix other stuff later.

Do you have remaining plans to do on this PR?

vm/src/protocol/number.rs Outdated

		@@ -0,0 +1,242 @@
		use std::borrow::Cow;

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

vm/src/builtins/int.rs Outdated

		@@ -263,7 +265,10 @@ impl Constructor for PyInt {
		val
		};

		try_int(&val, vm)
		// try_int(&val, vm)

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

// try_int(&val, vm)

vm/src/types/slot.rs Outdated

		pub trait AsNumber: PyPayload {
		#[inline]
		#[pyslot]
		fn slot_as_number(zelf: &PyObject, vm: &VirtualMachine) -> Cow<'static, PyNumberMethods> {

Copy link

Member

Choose a reason for hiding this comment

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

Does this slot need to return Cow? Which part prevent this to be&'static PyNumberMethods?

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

I think for heap object we need to use Cow::Owned witch implemented in as_number_wrapper

Copy link

Member

Choose a reason for hiding this comment

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

They are not decided by object, but decided by type. Then I think the type can hold a clonedPyNumberMethods with edited values. instead ofCow for every object whenas_number is called.

Copy link

Member

Choose a reason for hiding this comment

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

I found AsMapping has the same mechanism. Then it is ok for now.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Yes, it should be stored in the type struct, but we can fix it later

Copy link

ContributorAuthor

qingshi163 commentedMay 25, 2022

There is quite a lot of things not done yet, i think all the mathematic operations should be implemented via number protocol

Copy link

Member

youknowone commentedMay 25, 2022

How about finishing this PR as current state as core implementation of AsNumber trait, and working on other types in following other PRs?
Keeping a huge PR will be a burden to maintain (to rebase everything after other merge)

Copy link

Member

youknowone commentedMay 26, 2022

@qingshi163 are you going to make this ready for review? or do you want to do more work on it?

Copy link

ContributorAuthor

qingshi163 commentedMay 27, 2022

I am still busy with it

qingshi163 force-pushed thenumber-protocol branch fromf1bcb2e to4da2080Compare

May 28, 2022 19:47

youknowone mentioned this pull request

May 28, 2022

Implement protocols (Abstract obejcts layer)#3244

Open

qingshi163 marked this pull request as ready for review

May 29, 2022 06:53

qingshi163 requested a review fromyouknowone

May 29, 2022 06:53

youknowone changed the title~~[WIP] Implement Number Protocol~~Implement Number Protocol

May 29, 2022

youknowone requested changes

May 29, 2022

View reviewed changes

Copy link

Member

youknowone left a comment

Choose a reason for hiding this comment

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

I think#3750 changes also be able to be adapt here.
They can be done later, but I attached a commit simplifyingAsNumber interfaces.

vm/src/builtins/float.rs Outdated

		if let Some(f) = val.try_to_f64(vm)? {
		f
		if cls.is(vm.ctx.types.float_type) && val.class().is(PyFloat::class(vm)) {
		unsafe { val.downcast_unchecked::<PyFloat>().value }

Copy link

Member

youknowoneMay 29, 2022

Choose a reason for hiding this comment

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

here is a regression. val object will not be reused even for same type.

vm/src/builtins/float.rs OutdatedShow resolvedHide resolved

vm/src/builtins/int.rs Outdated

		negative: Some(\|number, vm\| {
		Self::number_downcast(number)
		.value
		.clone()

Copy link

Member

youknowoneMay 29, 2022

Choose a reason for hiding this comment

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

https://docs.rs/num/latest/num/struct.BigInt.html#impl-Neg-1

Neg is also implemented for&BigInt. This clone is redundant.

vm/src/builtins/int.rs Outdated

		absolute: Some(\|number, vm\| Self::number_downcast(number).value.abs().to_pyresult(vm)),
		boolean: Some(\|number, _vm\| Ok(Self::number_downcast(number).value.is_zero())),
		invert: Some(\|number, vm\| {
		let value = Self::number_downcast(number).value.clone();

Copy link

Member

youknowoneMay 29, 2022

Choose a reason for hiding this comment

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

Not also implemented for reference.

https://docs.rs/num/latest/num/struct.BigInt.html#impl-Not-1

vm/src/protocol/number.rs

Comment on lines +14 to +13

		/* Number implementations must check both
		arguments for proper type and implement the necessary conversions
		in the slot functions themselves. */

Copy link

Member

youknowoneMay 29, 2022

Choose a reason for hiding this comment

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

Does this mean we need to check the first argument(&PyNumber) is a correct PyNumber object?
In this case, I feel like the type will fit better in&PyObject rather than&PyNumber

Copy link

ContributorAuthor

qingshi163May 29, 2022

Choose a reason for hiding this comment

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

The comment is from cpython but as the source shows that cpython only does the check for binary operations which could be called invertly but assume the type is correct for other functions

vm/src/protocol/number.rs OutdatedShow resolvedHide resolved

Copy link

Member

youknowone commentedMay 29, 2022

+-----------+----------------------------------+-----------------------------------+----------------------------------------+| Benchmark | benches/out/cpython38/nbody.json | benches/out/rustpython/nbody.json | benches/out/rustpython-3507/nbody.json |+===========+==================================+===================================+========================================+| nbody     | 103 ms                           | 1.79 sec: 17.39x slower           | 1.82 sec: 17.70x slower                |+-----------+----------------------------------+-----------------------------------+----------------------------------------+

youknowone force-pushed thenumber-protocol branch from06a0d1c to1fd5d0aCompare

May 29, 2022 21:55

Copy link

Member

youknowone commentedMay 29, 2022

at least, no regression after using references

+-----------+----------------------------------+-----------------------------------+----------------------------------------+--------------------------------------------+| Benchmark | benches/out/cpython38/nbody.json | benches/out/rustpython/nbody.json | benches/out/rustpython-3507/nbody.json | benches/out/rustpython-3507/nbody-ref.json |+===========+==================================+===================================+========================================+============================================+| nbody     | 103 ms                           | 1.79 sec: 17.39x slower           | 1.82 sec: 17.70x slower                | 1.79 sec: 17.38x slower                    |+-----------+----------------------------------+-----------------------------------+----------------------------------------+--------------------------------------------+

PyNumber protocol

beeb449

youknowone force-pushed thenumber-protocol branch 2 times, most recently from1b9b663 to7a61d07Compare

May 29, 2022 22:34

qingshi163 added4 commits

May 30, 2022 07:36

Number protocol for PyInt

20632ed

deprecated warnings for int

59cedd2

impl number protocol float

3e6e348

fix vm.to_index now use number protocol

1ed18c0

youknowone force-pushed thenumber-protocol branch from7a61d07 to0c9d9a5Compare

May 29, 2022 22:37

qingshi163and others added2 commits

May 30, 2022 07:38

Simplify AsNumber trait

ea95777

Use static ref

09fc676

youknowone force-pushed thenumber-protocol branch from0c9d9a5 to09fc676Compare

May 29, 2022 22:38

youknowone approved these changes

May 29, 2022

View reviewed changes

vm/src/protocol/number.rs OutdatedShow resolvedHide resolved

youknowone merged commitc40bc62 intoRustPython:main

May 29, 2022

fanninpm mentioned this pull request

Jul 3, 2022

Implement Number protocol for PyComplex#3838

Merged

qingshi163 deleted the number-protocol branch

July 3, 2022 19:19

fanninpm mentioned this pull request

Jul 29, 2022

Unable to install RustPython from source with cargo install.#3982

Closed

Labels

None yet

2 participants

		absolute: Some(\|number, vm\| Self::number_downcast(number).value.abs().to_pyresult(vm)),
		boolean: Some(\|number, _vm\| Ok(Self::number_downcast(number).value.is_zero())),
		invert: Some(\|number, vm\| {
		let value = Self::number_downcast(number).value.clone();

Movatterモバイル変換

Implement Number Protocol#3507

Implement Number Protocol#3507

Uh oh!

Conversation

qingshi163 commentedDec 20, 2021• edited by youknowoneLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

youknowone commentedApr 26, 2022

Uh oh!

youknowone commentedMay 23, 2022

Uh oh!

youknowone left a comment• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qingshi163 commentedMay 25, 2022

Uh oh!

youknowone commentedMay 25, 2022

Uh oh!

youknowone commentedMay 26, 2022

Uh oh!

qingshi163 commentedMay 27, 2022

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

youknowone commentedMay 29, 2022

Uh oh!

youknowone commentedMay 29, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qingshi163 commentedDec 20, 2021•
edited by youknowone
Loading

youknowone left a comment•
edited
Loading