Skip to content

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