Skip to content

Refactor Number Protocol BinaryOps & HeapTypeExt #4720

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 16 commits into from

Conversation

qingshi163
Copy link
Contributor

No description provided.

@qingshi163
Copy link
Contributor Author

#4682

@qingshi163 qingshi163 changed the title [WIP] Refactor Number Protocol BinaryOps Refactor Number Protocol BinaryOps & HeapTypeExt Mar 20, 2023
@qingshi163 qingshi163 marked this pull request as ready for review March 20, 2023 19:32
@qingshi163 qingshi163 requested a review from youknowone March 20, 2023 19:32
@youknowone
Copy link
Member

@xiaozhiyan Could you review this PR?

@youknowone
Copy link
Member

and also @Snowapril

@@ -41,7 +41,7 @@ pub struct PyTypeSlots {
// Methods to implement standard operations

// Method suites for standard classes
pub as_number: AtomicCell<Option<PointerSlot<PyNumberMethods>>>,
pub as_number: Option<&'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.

Because every PyPayload has PyNumberMethods - or even NOT_IMPLEMENTED table, this field can be non-optional.

Suggested change
pub as_number: Option<&'static PyNumberMethods>,
pub as_number: &'static PyNumberMethods,

Comment on lines 31 to 37
if let Some(ext) = class.heaptype_ext.as_ref() {
ext.number_slots.$y.load()
} else if let Some(methods) = class.slots.as_number {
methods.$x
} else {
None
}
Copy link
Member

@youknowone youknowone Mar 21, 2023

Choose a reason for hiding this comment

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

can this code be simply like this?

        class.slots.as_number().$x

as_number always can exist. when number is not implemented, it can be PyNumberMethods::NOT_IMPLEMENTED.
when this is a python type, it can be a new PyNumberMethods::PYTHON_WRAPPER which holds wrapper functions to call binary ops magic methods.
We don't need to check if it is heaptype or not, becasue PyNumberMethods::PYTHON_WRAPPER will only be set for heaptypes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The different is as_number will point to a PyNumberMethods witch handles the right_* functions in a same way, but the heaptype hold a PyNumberSlots witch override right_* functions seperated.

The design differ from cpython witch hold both PyNumberMethods on static or heaptype and then as_number is set to point to the right one.

Our design have advantage in our structure is now the PyNumberMethods can be immutable and only use for static, as_number can be immutable and will be override by PyNumberSlots if is heaptype. PyNumberSlots can contains more informations so we can override right_* functions seperately witch in cpython have to fallback both.

So this is like a two atomic read vs one branch, I did not notice the performance change on my own test, but maybe you can help with a better testing.

@youknowone

Copy link
Member

Choose a reason for hiding this comment

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

ah, yep, that was the reason of slot hitting pynumbermethods.

A bit disagreement for the performance advantage. Because we just can add slots to static types too on the top of this patch, I believe as_number can be fully immutable in either way. So it will be one atomic read + one non-atomic read vs one branch + one non-atomic read. So it will be atomic read vs branch problem. And the cost of branch is comparably going more expensive every year. (not sure in this case because branchless design is mostly about arithmetic calculation, not about memory read)

I wonder if either design fits better to fix TestChainMap::test_union_operators regression.

Copy link
Member

Choose a reason for hiding this comment

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

Thinking about this for a while, I'd like to support slot one for its simplicity unless this implementation has recognizable performance gain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I think need to close this pr as the memory saving for internal type should not matter. I will split the useful codes to other pr

Copy link
Member

Choose a reason for hiding this comment

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

I thought we only need to edit a little bit more from this PR.
Memory saving for instances means a lot, but not for types. Number of types are limited and especially builtin types are more.

Comment on lines +51 to +58
if let Some(ext) = class.heaptype_ext.as_ref() {
if ext.number_slots.int.load().is_some()
|| ext.number_slots.index.load().is_some()
|| ext.number_slots.float.load().is_some()
{
return true;
}
}
Copy link
Member

@youknowone youknowone Mar 21, 2023

Choose a reason for hiding this comment

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

then we also don't need this check
oh, maybe not. because wrapper will always contains function.

return true;
}
}
obj.payload_is::<PyComplex>()
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
obj.payload_is::<PyComplex>()
false

this special type checking was added when complex didn't implement AsNumber

Copy link
Contributor Author

Choose a reason for hiding this comment

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

complex has AsNumber but did not implement any of int(), index() or float(), so it will fail. This is workaround is also exist in cpython.

Copy link
Member

Choose a reason for hiding this comment

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

I now remember you explained the same thing before 😅

@qingshi163 qingshi163 closed this Mar 21, 2023
@qingshi163 qingshi163 mentioned this pull request Mar 21, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants