Skip to content

gh-132284: Don't wrap base PyCFunction slots on class creation if not overridden #132329

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

Merged
merged 9 commits into from
Apr 17, 2025

Conversation

tom-pytel
Copy link
Contributor

@tom-pytel tom-pytel commented Apr 9, 2025

Will not wrap slot functions from base classes if they are not explicitly overridden on class creations.

Subclass __getitem__() performance even increases a bit from:

$ ./python -m timeit -s $'class d(dict): pass\ni = d({1:2})' "i[1]"
10000000 loops, best of 5: 28.5 nsec per loop

to:

$ ./python -m timeit -s $'class d(dict): pass\ni = d({1:2})' "i[1]"
10000000 loops, best of 5: 21.6 nsec per loop

Rationale for the checks is as follows:

  • generic == NULL because not sure of behavior with slots that loop several times like tp_getattr, tp_setattr and tp_richcompare.
  • Py_IS_TYPE(descr, &PyMethodDescr_Type) because only if setting pre-existing builtin C method.
  • *ptr == ((PyMethodDescrObject *)descr)->d_method->ml_meth checks both *ptr != NULL and stuff like:
class C:
    __new__ = type.__dict__['__subclasscheck__']

@tom-pytel tom-pytel changed the title Don't wrap base PyCFunction slots on class creation if not overridden gh-132284: Don't wrap base PyCFunction slots on class creation if not overridden Apr 9, 2025
@brandtbucher
Copy link
Member

Do we have a test that something like this continues to work correctly? If not, maybe we should add one:

>>> class D(dict):
...     pass
...     
>>> d = D({None: None})
>>> assert d[None] is None
>>> D.__getitem__ = lambda self, item: 42
>>> assert d[None] == 42

@iritkatriel
Copy link
Member

This is probably worth an entry in "what's new" in case someone notices something change for some edge case.

@iritkatriel
Copy link
Member

Also note merge conflict.

@tom-pytel
Copy link
Contributor Author

This is probably worth an entry in "what's new" in case someone notices something change for some edge case.

Have a look at addition to whatsnew. Not sure if is right place so if you want something else then tell me what to put where or feel free to change as needed.

@iritkatriel iritkatriel merged commit a23ed8b into python:main Apr 17, 2025
42 checks passed
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.

3 participants