Skip to content

gh-91603: Speed up UnionType instantiation (alt)#91955

Merged
serhiy-storchaka merged 2 commits intopython:mainfrom
serhiy-storchaka:faster-union-instance
Apr 28, 2022
Merged

gh-91603: Speed up UnionType instantiation (alt)#91955
serhiy-storchaka merged 2 commits intopython:mainfrom
serhiy-storchaka:faster-union-instance

Conversation

@serhiy-storchaka
Copy link
Copy Markdown
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
Copy Markdown
Member Author

The new code is even shorter than the old one!

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

@uriyyo
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

🤖 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