Speed symbol state merging back up #15731
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dcreager
requested review from
carljm,
MichaReiser,
AlexWaygood and
sharkdp
as code owners
2% faster according to codspeed! 🎉 🌮 |
MichaReiser
approved these changes
Jan 24, 2025
| self.live_bindings = a.live_bindings.clone(); | ||
| self.live_bindings.union(&b.live_bindings); | ||
|
|
||
| // Invariant: These zips are well-formed since we maintain an invariant that all of our | ||
| // fields are sets/vecs with the same length. | ||
| let a = (a.live_bindings.iter()) | ||
| .zip(a.constraints) | ||
| .zip(a.constraints.iter_mut()) |
There was a problem hiding this comment.
A comment here would probably be useful. It wasn't immediately clear to me why we use a &mut here even with your explanation. Should we do the same in the other merge method and for `visibility_constraints?
There was a problem hiding this comment.
I think it won't be as beneficial for visibility_constraints, since those are already smaller. But I can check! (And I will add the comment)
There was a problem hiding this comment.
Yep as expected, it was a wash doing the same thing to visibility_constraints. (Those are 32-bit IDs, which are cheap to copy, and not 24-byte BitSets.)
|
Amazing, thank you!! |
dcreager
added a commit
that referenced
this pull request
Jan 27, 2025
* main: Run `cargo update` (#15769) [red-knot] Document public symbol type inferece (#15766) Update dawidd6/action-download-artifact action to v8 (#15760) Update NPM Development dependencies (#15758) Update pre-commit dependencies (#15756) Update dependency ruff to v0.9.3 (#15755) Update dependency mdformat-mkdocs to v4.1.2 (#15754) Update Rust crate uuid to v1.12.1 (#15753) Update Rust crate unicode-ident to v1.0.15 (#15752) Fix docstring in ruff_annotate_snippets (#15748) Update Rust crate insta to v1.42.1 (#15751) Update Rust crate clap to v4.5.27 (#15750) Add references to `trio.run_process` and `anyio.run_process` (#15761) [`ruff`] Do not emit diagnostic when all arguments to `zip()` are variadic (`RUF058`) (#15744) [red-knot] Ensure differently ordered unions are considered equivalent when they appear inside tuples inside top-level intersections (#15743) [red-knot] Ensure differently ordered unions and intersections are understood as equivalent even inside arbitrarily nested tuples (#15740) [red-knot] Promote the `all_type_pairs_are_assignable_to_their_union` property test to stable (#15739) [`pylint`] Do not trigger `PLR6201` on empty collections (#15732) Improve the file watching failure error message (#15728) Speed symbol state merging back up (#15731)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow-up to #15702 that hopefully claws back the 1% performance regression. Assuming it works, the trick is to iterate over the constraints vectors via mut reference (aka a single pointer), so that we're not copying
BitSets into and out of the zip tuples as we iterate. We usestd::mem::takeas a poor-man's move constructor only at the very end, when we're ready to emplace it into the result. (C++ idioms intended! 😄)With local testing via hyperfine, I'm seeing this be 1-3% faster than
mainmost of the time — though a small number of runs (1 in 10, maybe?) are a wash or havemainfaster. Fingers crossed to see what codspeed says!