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

[red-knot] Use BitSet::union for merging of declarations #15451

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

sharkdp
Copy link
Contributor

@sharkdp sharkdp commented Jan 13, 2025

Summary

In SymbolState merging, use BitSet::union instead of inserting declarations one by one. This used to be the case but was changed in #15019 because we had to iterate over declarations anyway.

This is an alternative to #15419 by @MichaReiser, just to see which is better in terms of performance.

Codspeed results

There's almost no difference in performance (note that the baseline also changed):

image

image

Test Plan

In `SymbolState` merging, use `BitSet::union` instead of inserting
declarations one by one. This use to case but was changed in
#15019 because we had to iterate
over declarations anyway.
@sharkdp sharkdp added the red-knot Multi-file analysis & type inference label Jan 13, 2025
Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Copy link
Member

@MichaReiser MichaReiser 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 is the proper solution which I think is also easier to understand (it's clear that the order isn't relevant).

@sharkdp sharkdp merged commit eb3cb8d into main Jan 13, 2025
21 checks passed
@sharkdp sharkdp deleted the david/use-bitset-union-for-declarations branch January 13, 2025 10:10
Comment on lines +96 to +97
/// Union in-place with another [`BitSet`].
pub(super) fn union(&mut self, other: &BitSet<B>) {
Copy link
Member

Choose a reason for hiding this comment

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

nit that is probably not even worth a followup PR: I'd have probably called this something like union_with (similar to https://docs.rs/bit-set/latest/bit_set/struct.BitSet.html#method.union_with) since union methods on other structs seem to generally return a new value rather than working in-place (https://doc.rust-lang.org/std/collections/struct.HashSet.html#method.union, https://docs.rs/bitflags/latest/bitflags/trait.Flags.html#method.union, https://docs.rs/bit-set/latest/bit_set/struct.BitSet.html#method.union)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I just put some previously-approved code back in place that was there before, which is why I didn't wait for any other reviews. But that seems like a reasonable improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants