[ty] Improve union builder performance#22048
Conversation
MichaReiser
added
internal
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
| if element_td == ty_td { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This check is redundant with the check on line 577
CodSpeed Performance ReportMerging #22048 will improve performances by 19.12%Comparing Summary
Benchmarks breakdown
Footnotes
|
|
The memory and performance improvements make me a little suspicious. But maybe it's because we now defer computing |
| let mut remove_element = |i: &mut usize, elements: &mut Vec<UnionElement<'db>>| { | ||
| if inserted { | ||
| elements.swap_remove(*i); | ||
| } else { | ||
| elements[*i] = UnionElement::Type(ty); | ||
| *i += 1; | ||
| } | ||
| inserted = true; |
There was a problem hiding this comment.
Hmm, too bad. This is actually incorrect. But I'm surprised that there isn't a single failing test!
The issue is that we now insert the element even in case where it later turns out that it's redundant?
We also end up removing existing elements if ty turns out to be redundant (although, not sure when this would happen because it would mean another existing element has been redundant too?
There was a problem hiding this comment.
Funny, that this was the main performance improvement (up to 10%). Sort of confusing why that would be
| if insertion_point.is_none() { | ||
| insertion_point = Some(i); | ||
| } else { | ||
| elements.swap_remove(i); |
There was a problem hiding this comment.
Unlike before, the new implementation removes redundant elements before we decided if we want to add ty.
I think this might be okay because we only early-exit if the type is redundant with any existing type and, if that's the case, then the element that would be removed here must have been redundant too? But not feeling a 100% sure about this
There was a problem hiding this comment.
I think this rationale makes sense, yes. And the fact that all our tests pass (plus no ecosystem impact) gives me confidence that this is correct.
There was a problem hiding this comment.
I think that's correct, as long as our is-redundant-with implementation obeys transitivity. A comment might be good.
There was a problem hiding this comment.
Turns out, this assumption is incorrect or there is indeed a bug in our is_redundant_with. I added assert statements to all our early returns, asserting that insertion_point.is_none (meaning, we haven't inserted the element yet), and pep695_type_aliases.… - PEP 695 type aliases - Cyclic aliases - Recursive invariant now panics
MichaReiser
changed the title
MichaReiser
added
performance
| } | ||
| if existing.is_subtype_of(self.db, ty) { | ||
| to_remove = Some(index); | ||
| continue; |
There was a problem hiding this comment.
I think we should continue here, the same as in push_type?
There was a problem hiding this comment.
yes, I think this is correct
MichaReiser
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
| let mut found = None; | ||
| let mut to_remove = None; | ||
| let ty_negated = ty.negate(self.db); | ||
| let mut ty_negated = None; |
There was a problem hiding this comment.
did you consider using a OnceCell here? It might make the logic more readable and less repetitive
There was a problem hiding this comment.
Not sure what the benefit of using OnceCell here is. The main advantage of OnceCell is that it doesn't require mut, but the mut isn't an issue here.
There was a problem hiding this comment.
Thanks, yeah, a OnceCell doesn't really make much sense here. The main thing I was wondering about was whether there was any way to reduce the repetition of having to do ty_negated.get_or_insert_with(|| ty.negate(db)) so many times. See #22082.
| @@ -383,8 +383,10 @@ impl<'db> UnionBuilder<'db> { | |||
| } | |||
| if existing.is_subtype_of(self.db, ty) { | |||
There was a problem hiding this comment.
I had to play around with this code a little to figure out how we even get to this branch, and why it's correct here for to_remove to be Option<usize> rather than Vec<usize>. It might be helpful to add a comment here (and similar comments to the if existing.is_subtype_of(db, ty) calls in the Type::IntLiteral() and Type::BytesLiteral branches below):
| if existing.is_subtype_of(self.db, ty) { | |
| // e.g. `existing` could be `Literal[""] & Any`, | |
| // and `ty` could be `Literal[""]` | |
| if existing.is_subtype_of(self.db, ty) { |
There was a problem hiding this comment.
Yes I agree those comments would be helpful.
I think the reason it's OK to just track one to_remove is also a bit subtle -- it's because there are a limited number of possible subtypes of a literal, and all the possible subtypes (e.g. Literal[1] & Any, Literal[1] & Unknown) are also redundant with each other, so it's not possible that we'd have more than one as an existing element.
| } | ||
| if existing.is_subtype_of(self.db, ty) { | ||
| to_remove = Some(index); | ||
| continue; |
There was a problem hiding this comment.
yes, I think this is correct
| if insertion_point.is_none() { | ||
| insertion_point = Some(i); | ||
| } else { | ||
| elements.swap_remove(i); |
There was a problem hiding this comment.
I think this rationale makes sense, yes. And the fact that all our tests pass (plus no ecosystem impact) gives me confidence that this is correct.
|
If you're looking for further improvements to union-building performance: we've known for a while that we need to do the same thing for enum-literal types that we do for bytes-literal, int-literal and string-literal types. Big unions of enum literals are currently very slow. |
|
I'd feel slightly more comfortable if @carljm at least skimmed over the change, given that our mypy primer results aren't as useful anymore (are there changes? I can't tell) |
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Looks good, just one question I don't understand
| @@ -383,8 +383,10 @@ impl<'db> UnionBuilder<'db> { | |||
| } | |||
| if existing.is_subtype_of(self.db, ty) { | |||
There was a problem hiding this comment.
Yes I agree those comments would be helpful.
I think the reason it's OK to just track one to_remove is also a bit subtle -- it's because there are a limited number of possible subtypes of a literal, and all the possible subtypes (e.g. Literal[1] & Any, Literal[1] & Unknown) are also redundant with each other, so it's not possible that we'd have more than one as an existing element.
| while i < self.elements.len() { | ||
| let element = &mut self.elements[i]; |
There was a problem hiding this comment.
Why is this safe? Can't we remove elements and end up going out of bounds here? (It seems maybe we can't, given it doesn't happen in ecosystem -- but I don't understand why)
There was a problem hiding this comment.
Because of the check on the line above. It checks i against the current length of self.elements in each iteration
| if insertion_point.is_none() { | ||
| insertion_point = Some(i); | ||
| } else { | ||
| elements.swap_remove(i); |
There was a problem hiding this comment.
I think that's correct, as long as our is-redundant-with implementation obeys transitivity. A comment might be good.
|
Curious to see what the pydantic benchmark number is after rebasing on main, now fa57253 has landed (I'm sure it's still an improvement, just curious how much of an improvement it is now) |
|
I did a local benchmark with a rebase on main and got a 21% improvement (#22052 (comment)) |
MichaReiser
force-pushed
the
micha/union-builder-simplify
branch
from
eecc160 to
3848ada
Compare
|
I reverted the |
4 participants
Summary
negatedas it often isn't even neededRemoveto_removewith eagerly removing elements (I don't feel a 100% sure about this change but there isn't a single failing test)