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 #91865

Closed
wants to merge 20 commits into from

Conversation

uriyyo
Copy link
Member

@uriyyo uriyyo commented Apr 23, 2022

#91603

Summary

Remove types.UnionType.__args__ recreation (flatten_args and dedup_and_flatten_args ).
Use the fact that in the case when union with another types.UnionType that union has deduplicated normalized __args__.

As a result complexity from O((M+N)^2) was reduced to O(M*N), where M - length of left union args, N - length of right union args.

Benchmarks:

Benchmark this main
int | float 67.5 ns 121 ns: 1.80x slower
int | (list | float) 153 ns 250 ns: 1.64x slower
(int | float) | list 142 ns 238 ns: 1.67x slower
(int | float) | (list | set) 253 ns 411 ns: 1.63x slower
(float | set) | int | int | (float | list) 450 ns 687 ns: 1.53x slower
float | set | int | int | float | list 416 ns 750 ns: 1.80x slower
Geometric mean (ref) 1.67x slower

For big unions:

Benchmark this main
bool | int | float | complex | str | bytes | bytearray | list | set | frozenset | dict | range | property | classmethod | staticmethod | Exception 1.38 us 4.08 us: 2.96x slower

@JelleZijlstra JelleZijlstra self-requested a review April 24, 2022 05:02
@Fidget-Spinner Fidget-Spinner self-requested a review April 24, 2022 09:15
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.

Thanks, this overall makes sense to me, but I found a few style issues and one bigger problem.

Objects/unionobject.c Outdated Show resolved Hide resolved
Objects/unionobject.c Outdated Show resolved Hide resolved
Objects/unionobject.c Outdated Show resolved Hide resolved
@@ -468,23 +547,12 @@ make_union(PyObject *args)
{
assert(PyTuple_CheckExact(args));

args = dedup_and_flatten_args(args);
Copy link
Member

Choose a reason for hiding this comment

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

We still need this logic for the callsite in union_getitem.

Consider this case:

>>> from typing import TypeVar
>>> T = TypeVar("T")
>>> (list[T] | list[int])
list[~T] | list[int]
>>> (list[T] | list[int])[int]
list[int]

If I'm reading your code correctly, it would no longer deduplicate the two members. It would be good to also add a test case based on this example.

We should probably put the deduping logic directly in union_getitem.

Copy link
Member Author

Choose a reason for hiding this comment

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

@JelleZijlstra Actually, we don't need it, as far as such case already handled by tests:

self.assertEqual((list[T] | list[S])[int, int], list[int])

That's because after parameter substitution new UnionType is recreated by reducing newargs using bitwise or operator:

res = PyTuple_GET_ITEM(newargs, 0);
Py_INCREF(res);
for (Py_ssize_t iarg = 1; iarg < nargs; iarg++) {
PyObject *arg = PyTuple_GET_ITEM(newargs, iarg);
Py_SETREF(res, PyNumber_Or(res, arg));
if (res == NULL) {
break;
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Oh good catch! That seems a bit inefficient too, but we don't need to fix that in this PR; in any case it's probably a less performance-critical path.

@AlexWaygood AlexWaygood added performance Performance or resource usage topic-typing labels Apr 25, 2022
uriyyo and others added 4 commits April 25, 2022 18:52
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Some nitpicks. Did not review the code deeply yet.

Objects/unionobject.c Outdated Show resolved Hide resolved
Objects/unionobject.c Outdated Show resolved Hide resolved
Objects/unionobject.c Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

uriyyo and others added 3 commits April 26, 2022 11:42
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Objects/unionobject.c Outdated Show resolved Hide resolved
Objects/unionobject.c Outdated Show resolved Hide resolved
@uriyyo
Copy link
Member Author

uriyyo commented Apr 26, 2022

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@JelleZijlstra, @serhiy-storchaka: please review the changes made to this pull request.

Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

LGTM in general, I just have one question.

Objects/unionobject.c Outdated Show resolved Hide resolved
Objects/unionobject.c Show resolved Hide resolved
Objects/unionobject.c Show resolved Hide resolved
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, except _PyTuple_Resize and some formatting.

But the code can be much smaller. I have refactored it in #91955.

Objects/unionobject.c Outdated Show resolved Hide resolved
Objects/unionobject.c Outdated Show resolved Hide resolved
Objects/unionobject.c Outdated Show resolved Hide resolved
Objects/unionobject.c Outdated Show resolved Hide resolved
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

Although I prefer #91955, which is simpler. 😉

@uriyyo
Copy link
Member Author

uriyyo commented Apr 28, 2022

Closed because #91955 merged

@uriyyo uriyyo closed this Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants