Skip to content

Conversation

serhiy-storchaka
Copy link
Member

Co-authored-by: Yurii Karabas 1998uriyyo@gmail.com

It is a rewriting of #91865.

Closes #91603.

Co-authored-by: Yurii Karabas <1998uriyyo@gmail.com>
@serhiy-storchaka serhiy-storchaka added type-feature A feature request or enhancement performance Performance or resource usage labels Apr 26, 2022
@serhiy-storchaka serhiy-storchaka changed the title gh-91603: Speed up UnionType instantiation gh-91603: Speed up UnionType instantiation (alt) Apr 26, 2022
@serhiy-storchaka serhiy-storchaka added topic-typing and removed type-feature A feature request or enhancement labels Apr 26, 2022
@serhiy-storchaka serhiy-storchaka requested a review from uriyyo April 26, 2022 12:07
@serhiy-storchaka
Copy link
Member Author

The new code is even shorter than the old one!

 1 file changed, 77 insertions(+), 87 deletions(-)

@uriyyo
Copy link
Member

uriyyo commented Apr 26, 2022

@serhiy-storchaka Just compare performance with my version. It's almost identical:

Benchmark yurii serhiy
int | float 68.8 ns 73.2 ns: 1.06x slower
int | (list | float) 136 ns 139 ns: 1.02x slower
float | set | int | int | float | list 307 ns 288 ns: 1.06x faster
bool | int | float | complex | str | bytes | bytearray | list | set | frozenset | dict | range | property | classmethod | staticmethod | Exception 1.41 us 1.61 us: 1.14x slower
Geometric mean (ref) 1.02x slower

Benchmark hidden because not significant (3): (int | float) | list, (int | float) | (list | set), (float | set) | int | int | (float | list)

Copy link
Member

@uriyyo uriyyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@serhiy-storchaka
Copy link
Member Author

I am not surprised because it is the same algorithm.

"type | type" may be slightly slower because the code is more general, but it is a honest price for simpler code. On other hand, my code avoid creation a new tuple and a new UnionType object if possible, so it is expected that cases like float | set | int | int | float | list may be slightly faster.

I was skeptical about advisability of such optimization at all, but if it also reduces the size of the code I have no doubts.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good, but I have a question about refcounts.

if (args == NULL) {
return NULL;
if (*obj == Py_None) {
*obj = (PyObject *)&_PyNone_Type;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to INCREF here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it is a borrowed reference.

@JelleZijlstra JelleZijlstra added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 27, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @JelleZijlstra for commit a4badbc 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 27, 2022
@serhiy-storchaka serhiy-storchaka merged commit cd1fbbc into python:main Apr 28, 2022
@serhiy-storchaka serhiy-storchaka deleted the faster-union-instance branch April 28, 2022 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance or resource usage topic-typing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Speed up isinstance on union types
4 participants