[red-knot] Use BitSet::union for merging of declarations#15451
Conversation
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.
|
MichaReiser
left a comment
There was a problem hiding this comment.
Thanks. This is the proper solution which I think is also easier to understand (it's clear that the order isn't relevant).
| /// Union in-place with another [`BitSet`]. | ||
| pub(super) fn union(&mut self, other: &BitSet<B>) { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
Summary
In
SymbolStatemerging, useBitSet::unioninstead 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):
Test Plan
—