Conversation
| }, | ||
| declarations: SymbolDeclarations { | ||
| live_declarations: self.declarations.live_declarations.clone(), | ||
| live_declarations: Declarations::default(), |
There was a problem hiding this comment.
Could we initialize all vecs here with their existing capacity or even use the max capacity between a and b? It seems to me that we always add at least all existing entries.
There was a problem hiding this comment.
Note that live_declarations is a BitSet, not a vector. Or do you think it would make sense to add something like a capacity parameter for large BitSets that have overflowed onto the heap?
There was a problem hiding this comment.
Isn't the bitset dynamically sized? I'd suggest to have a BitSet::with_capacity so that it can allocate the right inner vector. We could consider doing the same for all other fields. E.g. visibility_constraint's length is at least as large as the max of a's and b's visibility constraints (if I understand the logic correctly)
There was a problem hiding this comment.
Isn't the bitset dynamically sized?
It is, I was mostly just responding to the "initialize all vecs here".
For small sizes, the BitSet is stack-allocated with a fixed capacity, so as long as we're below that threshold, reserving wouldn't help. But in general, adding reserve/with_capacity calls here sounds like a reasonable thing to try!
|
|
I'm not the expert in this area, but if this is correct, you could further simplify it: --- a/crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs
+++ b/crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs
@@ -127,7 +127,7 @@ type VisibilityConstraintsIntoIterator = smallvec::IntoIter<InlineVisibilityCons
/// Live declarations for a single symbol at some point in control flow, with their
/// corresponding visibility constraints.
-#[derive(Clone, Debug, PartialEq, Eq)]
+#[derive(Clone, Debug, PartialEq, Eq, Default)]
pub(super) struct SymbolDeclarations {
/// [`BitSet`]: which declarations (as [`ScopedDefinitionId`]) can reach the current location?
pub(crate) live_declarations: Declarations,
@@ -177,7 +177,7 @@ impl SymbolDeclarations {
/// Live bindings for a single symbol at some point in control flow. Each live binding comes
/// with a set of narrowing constraints and a visibility constraint.
-#[derive(Clone, Debug, PartialEq, Eq)]
+#[derive(Clone, Debug, PartialEq, Eq, Default)]
pub(super) struct SymbolBindings {
/// [`BitSet`]: which bindings (as [`ScopedDefinitionId`]) can reach the current location?
live_bindings: Bindings,
@@ -244,7 +244,7 @@ impl SymbolBindings {
}
}
-#[derive(Clone, Debug, PartialEq, Eq)]
+#[derive(Clone, Debug, PartialEq, Eq, Default)]
pub(super) struct SymbolState {
declarations: SymbolDeclarations,
bindings: SymbolBindings,
@@ -303,18 +303,7 @@ impl SymbolState {
b: SymbolState,
visibility_constraints: &mut VisibilityConstraints,
) {
- let mut a = Self {
- bindings: SymbolBindings {
- live_bindings: Bindings::default(),
- constraints: ConstraintsPerBinding::default(),
- visibility_constraints: VisibilityConstraintPerBinding::default(),
- },
- declarations: SymbolDeclarations {
- live_declarations: Declarations::default(),
- visibility_constraints: VisibilityConstraintPerDeclaration::default(),
- },
- };
-
+ let mut a = Self::default();
std::mem::swap(&mut a, self);It looks like it's a 1% speedup on the cold benchmark: https://codspeed.io/astral-sh/ruff/branches/micha%2Fdont-clone-live-declarations |
Yes, thank you. This is an oversight by me from #15019. There is no semantic difference, we just inserted declarations twice into the Your solution here might be the best way, but for comparison, I opened #15451 which changes the code back to the state before statically-known branches. It uses |
## 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. It's similar in performance, but a bit more declarative and less imperative.
Summary
I don't know what I'm doing but all tests still pass ;)
The declarations get merged below so maybe this is no longer needed?
Test Plan
cargo test