Skip to content
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

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

Merged
merged 2 commits into from
Apr 28, 2022

Conversation

serhiy-storchaka
Copy link
Member

Co-authored-by: Yurii Karabas [email protected]

It is a rewriting of #91865.

Closes #91603.

@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
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

Objects/unionobject.c Outdated Show resolved Hide resolved
@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