gh-149180: Avoid double checking tp_as_number, tp_as_sequence, tp_as_mapping#149317
gh-149180: Avoid double checking tp_as_number, tp_as_sequence, tp_as_mapping#149317anujbharambe wants to merge 8 commits intopython:mainfrom
Conversation
Documentation build overview
8 files changed ·
|
| if (f != NULL) | ||
| return sequence_repeat(f, v, w); | ||
| } | ||
| else if (mw != NULL) { |
There was a problem hiding this comment.
I don't understand this branch change.
There was a problem hiding this comment.
Updated, the new version removes all dead NULL checks on tp_as_number, tp_as_sequence, and tp_as_mapping throughout the file. They're now guaranteed non-NULL after PyType_Ready.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
| static PyNumberMethods _Py_empty_number_methods = {0}; | ||
| static PySequenceMethods _Py_empty_sequence_methods = {0}; | ||
| static PyMappingMethods _Py_empty_mapping_methods = {0}; |
There was a problem hiding this comment.
Those structs can be declared constants as well.
There was a problem hiding this comment.
Done, I have declared them static const.
| if (type->tp_as_mapping != NULL && base->tp_as_mapping != NULL) { | ||
| basebase = base->tp_base; | ||
| if (basebase->tp_as_mapping == NULL) | ||
| if (basebase == NULL || basebase->tp_as_mapping == NULL) |
There was a problem hiding this comment.
When is it possible for basebase to be NULL?
There was a problem hiding this comment.
basebase is base->tp_base, which is NULL when base is object (the root type has no parent). I have added a comment explaining this.
picnixz
left a comment
There was a problem hiding this comment.
Please make some benchmarks as well to see how much we gain (probably macro benchmarks from pyperformance for that).
I think there are other places where we have LHS/RHS being tested so it may require more than just the change in PyNumber_Multiply (e.g., C classes that have implement their own slots).
AFAIU, all tp_as_sequence are no more NULLs for PyNumber_InPlaceMultiply or am I wrong here?
|
Also, don't use LLMs for your PRs unless you at least disclose its usage and to understand what has been written by you or it. See our policy: https://devguide.python.org/getting-started/generative-ai/ |
NekoAsakura
left a comment
There was a problem hiding this comment.
Please read requirements in the issue carefully.
| return sequence_repeat(f, v, w); | ||
| } | ||
| else if (mw != NULL) { | ||
| if (mw != NULL) { |
There was a problem hiding this comment.
This would remove the need for the extra check
This is a dead check, exactly the kind of "extra check" the issue is asking to remove. There's a fair bit more like it that's been left in.
There was a problem hiding this comment.
Thanks for catching this. The latest push removes all dead NULL checks across the affected files.
| static PyNumberMethods _Py_empty_number_methods = {0}; | ||
| static PySequenceMethods _Py_empty_sequence_methods = {0}; | ||
| static PyMappingMethods _Py_empty_mapping_methods = {0}; |
There was a problem hiding this comment.
Immutable types can all share common constant structs, so this should use very little extra memory.
…tp_as_mapping During PyType_Ready, assign NULL tp_as_number, tp_as_sequence, and tp_as_mapping pointers to shared static const empty (all-zero) structs. After initialization, these three fields are guaranteed non-NULL for all ready types. Remove redundant NULL checks at all callsites across abstract.c, object.c, bytesobject.c, complexobject.c, floatobject.c, typeobject.c, _bisectmodule.c, and pycore_abstract.h.
2abdd22 to
b349bd7
Compare
|
I recommend adding assertions to guarantee |
|
|
|
Sorry, I meant for cases like |
|
This is already covered by assertions I added in |
|
Maybe it is worth to add macro like @picnixz WDYT? |
|
@picnixz Benchmarks as requested. Benchmark results (pyperformance)Ran 10 benchmarks on macOS (Apple Silicon, arm64, 12 logical CPUs). No measurable regression. All differences are within noise (<1%).
|
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @picnixz: please review the changes made to this pull request. |
gh-149180: Avoid double checking tp_as_number, tp_as_sequence, tp_as_mapping
During
PyType_Ready, assign NULLtp_as_number,tp_as_sequence, andtp_as_mappingpointers to shared static empty (all-zero) structs. After initialization, these three fields are guaranteed non-NULL for all ready types.This eliminates the need for callers to double-check — first the struct pointer, then the slot within — e.g.:
// Before: two checks needed
if (Py_TYPE(s)->tp_as_mapping && Py_TYPE(s)->tp_as_mapping->mp_length)
// After: only the slot check is needed (callsite cleanup is a follow-up)
if (Py_TYPE(s)->tp_as_mapping->mp_length)
Changes
Objects/typeobject.c
staticzero-initialized empty structs (_Py_empty_number_methods,_Py_empty_sequence_methods,_Py_empty_mapping_methods).type_ready_inherit(), aftertype_ready_inherit_as_structs(), assign the empty structs as fallbacks whentp_as_number,tp_as_sequence, ortp_as_mappingis still NULL. Placed outside theif (base != NULL)block so it coversPyBaseObject_Type.CHECK()assertions in_PyType_CheckConsistency()to enforce the non-NULL invariant for ready types.basebase == NULLguards ininherit_slots()before dereferencingbasebase->tp_as_number,basebase->tp_as_sequence, andbasebase->tp_as_mapping. Previously these were unreachable because the outerifcheckedbase->tp_as_number != NULLwhich was false forobject. Now that the empty struct makes it non-NULL,base->tp_base(NULL forobject) must be guarded.Objects/abstract.c
PyNumber_InPlaceMultiply, changedelse if (mw != NULL)toif (mw != NULL)in the sequence-repeat fallback. Previouslyint'stp_as_sequencewas NULL, so the first branch was skipped and theelse ifhandled the right operand (e.g.2 *= [1]). Now thatinthas a non-NULL emptytp_as_sequence, the first branch is entered but finds no slots; removing theelseallows the second branch to still run.Safety
basebaseNULL guards preserve existing behavior — all slots in the empty struct are NULL, soSLOTDEFINEDreturns false.tp_as_*(set bytype_new_alloc()). The fallback never triggers.tp_as_* != NULLguards at callsites: Become always-true for ready types, which is harmless. The inner slot check still correctly fails via the empty struct's NULL slots.cc- @markshannon