-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
@xiaozhiyan Could you review this PR? |
and also @Snowapril |
vm/src/types/slot.rs
Outdated
@@ -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>, |
There was a problem hiding this comment.
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.
pub as_number: Option<&'static PyNumberMethods>, | |
pub as_number: &'static PyNumberMethods, |
vm/src/protocol/number.rs
Outdated
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 | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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; | ||
} | ||
} |
There was a problem hiding this comment.
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>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
obj.payload_is::<PyComplex>() | |
false |
this special type checking was added when complex didn't implement AsNumber
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😅
No description provided.