gh-105250: Fix NEWOBJ handling of custom metaclasses in C pickle - #155920
Open
chaerrypick01 wants to merge 4 commits into
Open
gh-105250: Fix NEWOBJ handling of custom metaclasses in C pickle#155920chaerrypick01 wants to merge 4 commits into
chaerrypick01 wants to merge 4 commits into
Conversation
The NEWOBJ and NEWOBJ_EX opcodes are documented to call cls.__new__(cls, *args), but the C implementation called tp_new directly, so a metaclass __getattribute__ hook was skipped unless the class happened to define __new__ in Python. Perform a real attribute lookup of cls.__new__ when the class has a custom metaclass, matching the pure Python implementation. The default-metaclass case keeps calling tp_new directly, where the lookup is not observable.
corona10
reviewed
Aug 17, 2026
corona10
reviewed
Aug 17, 2026
| metaclass_new_lookups = [] | ||
|
|
||
| class LookupLoggingMeta(type): | ||
| def __getattribute__(cls, name): |
Member
There was a problem hiding this comment.
It's better to move look up count as class varaible
Something like:
class LookupLoggingMeta(type):
new_lookup_count = 0
def __getattribute__(cls, name):
Author
There was a problem hiding this comment.
Done in f22a1a5 — moved the count to a class variable on the metaclass.
Member
|
Can you also provide benchmark compare to main branch by using pyperf? |
…NaXOE.rst Co-authored-by: Donghee Na <donghee.na92@gmail.com>
corona10
reviewed
Aug 17, 2026
|
|
||
| def __getattribute__(cls, name): | ||
| if name == '__new__': | ||
| LookupLoggingMeta.new_lookup_count += 1 |
Member
There was a problem hiding this comment.
Suggested change
| LookupLoggingMeta.new_lookup_count += 1 | |
| cls.new_lookup_count += 1 |
?
corona10
reviewed
Aug 17, 2026
Comment on lines
+6339
to
+6358
| /* Look __new__ up on the class so that a custom metaclass | ||
| __getattribute__ observes the lookup, as in the Python | ||
| implementation. */ | ||
| PyObject *func = PyObject_GetAttr(cls, &_Py_ID(__new__)); | ||
| if (func == NULL) { | ||
| goto error; | ||
| } | ||
| Py_ssize_t nargs = PyTuple_GET_SIZE(args); | ||
| PyObject *newargs = PyTuple_New(nargs + 1); | ||
| if (newargs == NULL) { | ||
| Py_DECREF(func); | ||
| goto error; | ||
| } | ||
| PyTuple_SET_ITEM(newargs, 0, Py_NewRef(cls)); | ||
| for (Py_ssize_t i = 0; i < nargs; i++) { | ||
| PyTuple_SET_ITEM(newargs, i + 1, | ||
| Py_NewRef(PyTuple_GET_ITEM(args, i))); | ||
| } | ||
| obj = PyObject_Call(func, newargs, kwargs); | ||
| Py_DECREF(newargs); |
Member
There was a problem hiding this comment.
Suggested change
| /* Look __new__ up on the class so that a custom metaclass | |
| __getattribute__ observes the lookup, as in the Python | |
| implementation. */ | |
| PyObject *func = PyObject_GetAttr(cls, &_Py_ID(__new__)); | |
| if (func == NULL) { | |
| goto error; | |
| } | |
| Py_ssize_t nargs = PyTuple_GET_SIZE(args); | |
| PyObject *newargs = PyTuple_New(nargs + 1); | |
| if (newargs == NULL) { | |
| Py_DECREF(func); | |
| goto error; | |
| } | |
| PyTuple_SET_ITEM(newargs, 0, Py_NewRef(cls)); | |
| for (Py_ssize_t i = 0; i < nargs; i++) { | |
| PyTuple_SET_ITEM(newargs, i + 1, | |
| Py_NewRef(PyTuple_GET_ITEM(args, i))); | |
| } | |
| obj = PyObject_Call(func, newargs, kwargs); | |
| Py_DECREF(newargs); | |
| PyThreadState *tstate = _PyThreadState_GET(); | |
| obj = _PyObject_Call_Prepend(tstate, func, cls, args, kwargs); |
Can you benchmark this one too?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The NEWOBJ and NEWOBJ_EX opcodes are documented to call
cls.__new__(cls, *args), but the C implementation calledtp_newdirectly, so a metaclass__getattribute__hook was skipped unless the class happened to define__new__in Python. Perform a real attribute lookup ofcls.__new__when the class has a custom metaclass, matching the pure Python implementation.When the metaclass is exactly
type, no hook can exist and the lookup is not observable, sotp_newis still called directly - no performance change for ordinary classes.The new test runs against both implementations and covers NEWOBJ (protocols 2-5) and NEWOBJ_EX (protocols 4-5); the cases inheriting
object.__new__fail on the C unpickler without this change.I benchmarked the three affected paths with pyperf 2.10.0 (script below). Both interpreters were built with the same plain
./configure(release build,Py_DEBUG=0). Baseline ismainat 70fdc96. Each benchmark unpickles alist of 1000 instances.
newobj_default(NEWOBJ, default metaclass)newobj_ex_default(NEWOBJ_EX, default metaclass)newobj_metaclass(NEWOBJ, custom metaclass)The fast path shows no measurable regression for either opcode — the added
Py_TYPE(cls) == &PyType_Typecheck is lost in the noise. The 1.15x slowdown is confined to the new slow path, which is only taken for classes with a custom metaclass: the cost of thePyObject_GetAttr(cls, '__new__')call plus building the argument tuple, on the path where the previous behaviour did not match the documented semantics.Environment: macOS 26.5.1, Apple M3 Pro, no CPU isolation (
pyperf system tuneis Linux-only), 20 processes per benchmark.Given the trade-off — documented
cls.__new__semantics for custom-metaclass classes, at ~15% on that path and no change for ordinary classes — I'd like your feedback on whether this is acceptable, or whether you'd prefer to closegh-105250 as a known limitation instead.
Benchmark script