Repository navigation
[ty] Lazily initialize UnionBuilder when transforming a union type - #24929
Conversation
…een changed by a transformation
mtshiba
added
performance
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 89.36%. The percentage of expected errors that received a diagnostic held steady at 85.49%. The number of fully passing files held steady at 88/134. |
Memory usage reportSummary
Significant changesClick to expand detailed breakdownflake8
trio
sphinx
prefect
|
|
Merging this PR will improve performance by 16.14%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | ty_micro[typevar_mapping_accumulation] |
371.2 ms | 319.6 ms | +16.14% |
| ⚡ | WallTime | static_frame |
21.8 s | 20.6 s | +5.51% |
| ⚡ | WallTime | pandas |
64.2 s | 60.5 s | +6.2% |
| ⚡ | WallTime | altair |
5.1 s | 4.7 s | +8.84% |
| ⚡ | WallTime | multithreaded |
1.2 s | 1.1 s | +13.09% |
| ⚡ | WallTime | colour_science |
44.1 s | 38.5 s | +14.59% |
| ⚡ | Simulation | hydra-zen |
1.2 s | 1.1 s | +10.1% |
Comparing mtshiba:identity-preservation (23b60b3) with main (37ae52f)
Footnotes
-
60 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
mtshiba
changed the title
UnionBuilder if the elements of the union have not been changed by a transformationUnionBuilder when transforming a union type
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
Nice.
I'm a bit surprised by the change in memory usage. I would expect this not to change memory usage at all. Do you know where the difference is coming from?
| let elements = self.elements(db); | ||
| let mut builder: Option<UnionBuilder<'db>> = None; | ||
| for (i, ty) in elements.iter().enumerate() { | ||
| let new_ty = transform_fn(ty); | ||
| if let Some(b) = builder.as_mut() { | ||
| b.add_in_place(new_ty); | ||
| } else if &new_ty != ty { | ||
| // First change at index `i`. Initialize the builder and | ||
| // replay the unchanged prefix (we know `elements[..i]` | ||
| // mapped to themselves, so we pass the originals directly). | ||
| let mut b = UnionBuilder::new(db); | ||
| for prev in &elements[..i] { | ||
| b.add_in_place(*prev); | ||
| } | ||
| b.add_in_place(new_ty); | ||
| builder = Some(b); | ||
| } | ||
| } | ||
| let Some(builder) = builder else { | ||
| return Type::Union(self); | ||
| }; |
There was a problem hiding this comment.
Nit: You can avoid some of the branching here by rewriting this to:
let elements = self.elements(db);
let mut iter = elements.iter().enumerate();
while let Some((i, ty)) = iter.next() {
let new_ty = transform_fn(ty);
if &new_ty != ty {
// First change at index `i`. Initialize the builder and
// replay the unchanged prefix (we know `elements[..i]`
// mapped to themselves, so we pass the originals directly).
let mut b = UnionBuilder::new(db);
for prev in &elements[..i] {
b.add_in_place(*prev);
}
b.add_in_place(new_ty);
for (_, element) in iter {
b.add_in_place(transform_fn(element));
}
return b.recursively_defined(self.recursively_defined(db)).build();
}
}
return Type::Union(self);
| // mapped to themselves, so we pass the originals directly). | ||
| let mut b = UnionBuilder::new(db); | ||
| for prev in &elements[..i] { | ||
| b.add_in_place(*prev); |
There was a problem hiding this comment.
It would be nice to have a method here that adds the element without doing any merging.
| // First change at index `i`. Initialize the builder and | ||
| // replay the unchanged prefix (we know `elements[..i]` | ||
| // mapped to themselves, so we pass the originals directly). | ||
| let mut b = UnionBuilder::new(db); |
There was a problem hiding this comment.
Do we need to set recursively_defined here?
There was a problem hiding this comment.
I think no... because this builder doesn't use cycle recovery...
There was a problem hiding this comment.
Yeah, it seems to correctly preserve the existing behavior.
But I'm a bit surprised that we only set it at the end, once we fully build the union, even though recursively_defined is read in add_in_place (in which case it is always false, regardless of what self.recursively_defined was)
There was a problem hiding this comment.
This flag only changes the behavior when performing a union with the type of the previous iteration and the type of the current iteration during cycle recovery.
The reason for re-attaching the flag here is simply to prevent the flag that was attached to the original union from being lost.
|
I think we should revert this PR because of this codex finding (this PR is not behavior preserving)
and
|
|
I’ll revert. Those both seem fixable? So I can follow up on them separately! |
…on type" (#25049) Reverts #24929. See: #24929 (comment).
|
I'm working on a follow up fix, I think I'm close :) |
|
Thanks king! |
* main: (23 commits) Lazily initialize `UnionBuilder` when transforming a `Union` (#25050) [ty] Speed-up find-references by using multithreading for cross-file searches (#25042) Revert "[ty] Lazily initialize `UnionBuilder` when transforming a union type" (#25049) [ty] Make text-based find-references pre-filter identifier boundary aware (#25036) [ty] Fix cross-file find-references for keyword arguments (#25043) [ty] Include TypedDict type context when inferring mixed constructors (#25039) [ty] Include TypedDict type context when inferring string keys (#25037) [ty] Lazily initialize `UnionBuilder` when transforming a union type (#24929) [ty] Skip global search for references if identifier is not externally visible (#25033) [ty] Support narrowing on `__class__` checks (#24997) [ty] Bump ecosystem-analyzer (#25032) [ty] Include error context for overload consistency diagnostics (#24950) [ty] Check non-generic overload implementations (#24936) Filter overloads based on return type for ParamSpec mapping (#24769) [ty] Add versioned parameters for dataclass decorator (#25029) [ty] Project reachability constraints before narrowing (#24982) [ty] Emit folding ranges from the language server for multi-line block headers. (#24978) [ty] Allow ParamSpec specialization through unioned generic classes (#24826) [ty] Adjust start of block folding range to preserve visible header for character-precise LSP clients. (#24917) [ty] Use `as_i64` when comparing `IntLiteralType` instances (#25023) ...
|
I'm sorry I haven't been able to get to this for a while.
This isn't the kind of optimization that reduces query calls or increases cache hit rates, so the cause is probably the size of the query metadata.
It simply turned out to be not very effective.
The point that using this trick in Anyway, it shows that the main performance improvement from this change actually comes from |
…stral-sh#24929) ## Summary This early return improves performance. I tried the same thing at intersections, but there wasn't much change. It seems `UnionType::map_leave_aliases` was a particularly hotspot. ## Test Plan --------- Co-authored-by: Charlie Marsh <[email protected]>
4 participants
Summary
This early return improves performance.
I tried the same thing at intersections, but there wasn't much change. It seems
UnionType::map_leave_aliaseswas a particularly hotspot.Test Plan