[red-knot] Also use alists for list of live bindings/declarations#16311
[red-knot] Also use alists for list of live bindings/declarations#16311
Conversation
dcreager
added
performance
There was a problem hiding this comment.
Much of the churn is this file is from the new parameters to thread through the SymbolStates arenas
dcreager
requested review from
AlexWaygood,
MichaReiser,
carljm and
sharkdp
as code owners
|
|
(Haven't reviewed this one yet, just wanted to note that CodSpeed seems to think this one is a 1% cold-check regression. So if that's consistent, we'll need to decide if there are sufficient code simplicity arguments to do it anyway, or if there's something we can do to eliminate the regression.) |
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
The code changes look good to me (they seem mostly mechanical).
What would help me review this PR conceptually is a short explanation on how live bindings and declarations are used (what are the creation and access patterns) and why using alists is a good fit.
|
|
||
| /// Restore the current builder symbols state to the given snapshot. | ||
| pub(super) fn restore(&mut self, snapshot: FlowSnapshot) { | ||
| pub(super) fn restore(&mut self, snapshot: &FlowSnapshot) { |
There was a problem hiding this comment.
I prefer accepting own instances for functions that unconditionally clone the parameter to make the cost more visible to callers. It can also help avoiding unnecessary calls in the cases where doesn't use the owned instance besides calling the function
| .bindings | ||
| .iter(other.live_bindings) | ||
| .map(|(binding, _)| *binding); | ||
| if !self_bindings.eq(other_bindings) { |
There was a problem hiding this comment.
Is it possible to keep the len comparison to avoid comparing iterators of different lengths?
crates/ruff_index/src/list.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl<I: Idx, K: Clone, V> ListBuilder<I, K, V> { |
There was a problem hiding this comment.
Nit: I'd probably move the Clone constraint closer to map to avoid that we accidentally "inherit" it for other methods adding to this ListBuilder impl block
* main: (38 commits) [red-knot] Use arena-allocated association lists for narrowing constraints (#16306) [red-knot] Rewrite `Type::try_iterate()` to improve type inference and diagnostic messages (#16321) Add issue templates (#16213) Normalize inconsistent markdown headings in docstrings (#16364) [red-knot] Better diagnostics for method calls (#16362) [red-knot] Add argfile and windows glob path support (#16353) [red-knot] Handle pipe-errors gracefully (#16354) Rename `venv-path` to `python` (#16347) [red-knot] Fixup some formatting in `infer.rs` (#16348) [red-knot] Restrict visibility of more things in `class.rs` (#16346) [red-knot] Add diagnostic for class-object access to pure instance variables (#16036) Add `per-file-target-version` option (#16257) [PLW1507] Mark fix unsafe (#16343) [red-knot] Add a test to ensure that `KnownClass::try_from_file_and_name()` is kept up to date (#16326) Extract class and instance types (#16337) Re-order changelog entries for 0.9.7 (#16344) [red-knot] Add support for `@classmethod`s (#16305) Update Salsa (#16338) Update Salsa part 1 (#16340) Upgrade Rust toolchain to 1.85.0 (#16339) ...
* dcreager/dont-have-a-cow: Reuse unaliased cells in union Reuse unaliased cells in intersection Tracked aliased cells and update them in-place on insert Create structural property tests for union/intersection Add structural sharing property tests Don't clone any cells until we know we need to stitch up Add property tests Require explicit clones of alists Use named fields for list cells
* main: [red-knot] unify LoopState and saved_break_states (#16406) [`pylint`] Also reports `case np.nan`/`case math.nan` (`PLW0177`) (#16378) [FURB156] Do not consider docstring(s) (#16391) Use `is_none_or` in `stdlib-module-shadowing` (#16402) [red-knot] Upgrade salsa to include `AtomicPtr` perf improvement (#16398) [red-knot] Fix file watching for new non-project files (#16395) document MSRV policy (#16384) [red-knot] fix non-callable reporting for unions (#16387) bump MSRV to 1.83 (#16294) Avoid unnecessary info at non-trace server log level (#16389) Expand `ruff.configuration` to allow inline config (#16296) Start detecting version-related syntax errors in the parser (#16090)
* dcreager/dont-have-a-cow: [red-knot] unify LoopState and saved_break_states (#16406) [`pylint`] Also reports `case np.nan`/`case math.nan` (`PLW0177`) (#16378) [FURB156] Do not consider docstring(s) (#16391) Use `is_none_or` in `stdlib-module-shadowing` (#16402) [red-knot] Upgrade salsa to include `AtomicPtr` perf improvement (#16398) [red-knot] Fix file watching for new non-project files (#16395) document MSRV policy (#16384) [red-knot] fix non-callable reporting for unions (#16387) bump MSRV to 1.83 (#16294) Avoid unnecessary info at non-trace server log level (#16389) Expand `ruff.configuration` to allow inline config (#16296) Start detecting version-related syntax errors in the parser (#16090)
CodSpeed Performance ReportMerging #16311 will degrade performances by 4.18%Comparing Summary
Benchmarks breakdown
|
* dcreager/dont-have-a-cow: cargo shear clippy clippy
* dcreager/dont-have-a-cow: Add mutable list handle and `for_each`
Regardless of whether #16408 and #16311 pan out, this part is worth pulling out as a separate PR. Before, you had to define a new `IndexVec` index type for each type of association list you wanted to create. Now there's a single index type that's internal to the alist implementation, and you use `List<K, V>` to store a handle to a particular list. This also adds some property tests for the alist implementation.
|
I've tried a few variations on this, but have not been able to get a performance increase. I get things down to performance-neutral with the reusable alist cells from #16408, but that's not worth the extra code complexity. |
3 participants
This PR extends #16306 to also use association lists to store the list of live bindings and declarations for each symbol in the use-def map.