[issue45697] PyType_IsSubtype is doing excessive work in the common case

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

Thank you for your contribution Itamar.

--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45697] PyType_IsSubtype is doing excessive work in the common case

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:


New changeset 2c045bd5673d56c3fdde7536da9df1c7d6f270f0 by Itamar Ostricher in 
branch 'main':
bpo-45697: Use PyObject_TypeCheck in type_call (GH-29392)
https://github.com/python/cpython/commit/2c045bd5673d56c3fdde7536da9df1c7d6f270f0


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45697] PyType_IsSubtype is doing excessive work in the common case

[Itamar Ostricher]

Itamar Ostricher  added the comment:

thanks for the feedback Serhiy!

repeating my response from the PR here as well (not sure what's the proper 
etiquette.. :-) )

note that this code path is not for creating new types (which is slow as you 
say), but for creating new instances (which is quite fast).

I ran some synthetic benchmarks with this change and without it (on an opt 
build with PGO and LTO), and the gain is measurable:

build from main (commit acc89db9233abf4d903af9a7595a2ed7478fe7d3 which is the 
base commit for this PR):

```
>for _ in {1..10}; do ./main-opt/python -m timeit -n 1 -s 'class Spam: 
>pass' -- 'ten_k_spams = [Spam() for _ in range(1)]'; done
1 loops, best of 5: 896 usec per loop
1 loops, best of 5: 887 usec per loop
1 loops, best of 5: 857 usec per loop
1 loops, best of 5: 838 usec per loop
1 loops, best of 5: 847 usec per loop
1 loops, best of 5: 863 usec per loop
1 loops, best of 5: 845 usec per loop
1 loops, best of 5: 902 usec per loop
1 loops, best of 5: 890 usec per loop
1 loops, best of 5: 875 usec per loop
```

build with this change applied (commit 
2362bf67e8acee49c6f97ea754d59dfd8982e07c):

```
>for _ in {1..10}; do ./test-opt/python -m timeit -n 1 -s 'class Spam: 
>pass' -- 'ten_k_spams = [Spam() for _ in range(1)]'; done
1 loops, best of 5: 833 usec per loop
1 loops, best of 5: 885 usec per loop
1 loops, best of 5: 845 usec per loop
1 loops, best of 5: 838 usec per loop
1 loops, best of 5: 833 usec per loop
1 loops, best of 5: 827 usec per loop
1 loops, best of 5: 858 usec per loop
1 loops, best of 5: 811 usec per loop
1 loops, best of 5: 843 usec per loop
1 loops, best of 5: 845 usec per loop
```

also worth noting that the previous approach (GH-29380) was implemented on the 
Instagram Server production workload in Meta/Facebook/Instagram (choose your 
favorite) and resulted measurable perf gain of around 0.2%

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45697] PyType_IsSubtype is doing excessive work in the common case

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

Creating a new type takes microseconds, and using PyObject_TypeCheck() instead 
of PyType_IsSubtype() can save nanoseconds. So, while this replacement looks 
harmless, its effect can hardly be measured even in microbenchmarks.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45697] PyType_IsSubtype is doing excessive work in the common case

[Itamar Ostricher]

Itamar Ostricher  added the comment:

thanks for the feedback & discussion, I submitted a new PR

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45697] PyType_IsSubtype is doing excessive work in the common case

[Itamar Ostricher]

Change by Itamar Ostricher :


--
pull_requests: +27650
pull_request: https://github.com/python/cpython/pull/29392

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45697] PyType_IsSubtype is doing excessive work in the common case

[Ken Jin]

Ken Jin  added the comment:

> Dennis, you mean something like this? 
> https://github.com/itamaro/cpython/commit/92d46b260cf6ccce1a47003f539294530138e488

Not Dennis, but these changes looks good. As Serhiy mentioned, we can replace 
the hot callsites with PyObject_TypeCheck without slowing down PyType_IsSubtype.

--
nosy: +kj

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45697] PyType_IsSubtype is doing excessive work in the common case

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

The PyObject_TypeCheck() macro is used in performance critical cases. If it is 
not the case the PyType_IsSubtype() function can be used. Adding yet check in 
PyType_IsSubtype() will slow down more performance sensitive cases in which 
PyObject_TypeCheck() is used and speed up less performance sensitive cases. So 
there is no reason to add it.

--
nosy: +serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45697] PyType_IsSubtype is doing excessive work in the common case

[Oleg Iarygin]

Oleg Iarygin  added the comment:

Dennis, can PyFrozenSet_Check and _PyObject_TypeCheck get rid of Py_IS_TYPE 
invocation then, so PyType_IsSubtype becomes the only source of truth here?

--
nosy: +arhadthedev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45697] PyType_IsSubtype is doing excessive work in the common case

[Itamar Ostricher]

Itamar Ostricher  added the comment:

Dennis, you mean something like this? 
https://github.com/itamaro/cpython/commit/92d46b260cf6ccce1a47003f539294530138e488

sure, that would preempt the `PyType_IsSubtype` calls coming from these 
specific callsites, but the early return in `PyType_IsSubtype` would cover 
those as well as any other path, wouldn't it?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45697] PyType_IsSubtype is doing excessive work in the common case

[Dennis Sweeney]

Dennis Sweeney  added the comment:

Interesting. It seems like several call sites already check the equality case:

 setobject.h: 
#define PyFrozenSet_Check(ob) \
(Py_IS_TYPE(ob, _Type) || \
  PyType_IsSubtype(Py_TYPE(ob), _Type))

 object.h: 
static inline int _PyObject_TypeCheck(PyObject *ob, PyTypeObject *type) {
return Py_IS_TYPE(ob, type) || PyType_IsSubtype(Py_TYPE(ob), type);
}
#define PyObject_TypeCheck(ob, type) _PyObject_TypeCheck(_PyObject_CAST(ob), 
type)


Perhaps it would be better to use PyObject_TypeCheck where applicable (such as 
in the type_call function you mention, and in abstract.c's binary_op1()).

--
nosy: +Dennis Sweeney

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45697] PyType_IsSubtype is doing excessive work in the common case

[Itamar Ostricher]

Change by Itamar Ostricher :


--
keywords: +patch
pull_requests: +27638
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/29380

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45697] PyType_IsSubtype is doing excessive work in the common case

[Itamar Ostricher]

New submission from Itamar Ostricher :

Based on real world profiling data we collected, a vast amount of 
`PyType_IsSubtype` calls are coming from `type_call`, when it decides whether 
`__init__` should run or not.

In the common case, the arguments to this call are identical, but the 
implementation still walks the MRO.

By returning early for identical types, the common case can be optimized with a 
non-trivial performance gain.

--
components: Interpreter Core
messages: 405575
nosy: itamaro
priority: normal
severity: normal
status: open
title: PyType_IsSubtype is doing excessive work in the common case
type: performance
versions: Python 3.11

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com