[ty] Enable LRU collection for parsed module#21749
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
|
MichaReiser
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as outdated.
This comment was marked as outdated.
MichaReiser
requested review from
BurntSushi
and removed request for
AlexWaygood,
dcreager and
sharkdp
| thread_local! { | ||
| pub static TRACKER: RefCell<Option<StandardTracker>>= const { RefCell::new(None) }; | ||
| } |
There was a problem hiding this comment.
Would appreciate a comment explaining what on earth is going on here. Also, interesting that this no longer needs to be multi-threaded..?
There was a problem hiding this comment.
This was never multi-threaded. It's just that it was a static before, so we had to use a Mutex
| if !db.project().files(db).contains(&file) { | ||
| // Eagerly clear ASTs of third party files. | ||
| parsed.clear(); | ||
| } |
There was a problem hiding this comment.
Does this mean that each time completions are requested, every file will need to be re-parsed?
I checked out this PR and tried it out. I used a single Python source file containing just array. I opened it in VS Code and requested completions at the end of array. On main, I get:
2025-12-02 09:48:31.234246045 DEBUG request{id=13 method="textDocument/completion"}: Completions request returned 326 suggestions in 149.088978ms
2025-12-02 09:48:34.461071428 DEBUG request{id=15 method="textDocument/completion"}: Completions request returned 326 suggestions in 17.225306ms
2025-12-02 09:48:36.677139939 DEBUG request{id=17 method="textDocument/completion"}: Completions request returned 326 suggestions in 19.559503ms
2025-12-02 09:48:40.656242428 DEBUG request{id=19 method="textDocument/completion"}: Completions request returned 326 suggestions in 18.719669ms
And with your branch I get:
2025-12-02 09:50:26.152804396 DEBUG request{id=14 method="textDocument/completion"}: Completions request returned 326 suggestions in 166.780705ms
2025-12-02 09:50:29.762248593 DEBUG request{id=16 method="textDocument/completion"}: Completions request returned 326 suggestions in 19.985813ms
2025-12-02 09:50:33.345613225 DEBUG request{id=18 method="textDocument/completion"}: Completions request returned 326 suggestions in 19.429199ms
2025-12-02 09:50:37.048968976 DEBUG request{id=20 method="textDocument/completion"}: Completions request returned 326 suggestions in 18.763721ms
So there still seems to be significant caching happening?
There was a problem hiding this comment.
Does this mean that each time completions are requested, every file will need to be re-parsed?
We still cache symbols_for_file_global_only. But yes, we'll have to reparse the AST if a third party file changes, but that should be very unlikely and not something we can't really avoid because we obviously have to recompute symbols_for_file_global_only in that case.
The main drawback of this is if we had any other query that iterates over all third-party modules, parses them, and does stuff. But I don't think there's any such query?
There was a problem hiding this comment.
Ah I see now. Yeah this is great. And no, I'm not aware of any other queries that do this. I agree we'd have to be careful introducing one. Maybe add a comment here warning of that eventuality?
AlexWaygood
changed the title
We should still count the metadata. Maybe we want separate counts for memos with/without values? |
ibraheemdev
left a comment
ibraheemdev
left a comment
There was a problem hiding this comment.
We talked a bit about better LRU heuristics, and potentially changing completions to attempt to load the semantic index if it has already been created instead of reparsing the file, but this seems great for now!
|
For tomorrow, I should test that this doesn't regress multithreading performance in a meaningful way, because the LRU uses a single |
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
The memory impact here is amazing.
| /// | ||
| /// Prefer using [`symbol_table`] when working with symbols from a single scope. | ||
| #[salsa::tracked(returns(ref), no_eq, heap_size=ruff_memory_usage::heap_size)] | ||
| #[salsa::tracked(returns(ref), no_eq, heap_size=ruff_memory_usage::heap_size, lru=200)] |
There was a problem hiding this comment.
Similarly here, let's comment on what assumptions the 200 is based on.
|
Hmm, yeah. We can't land this. This is a huge perf regression. Checking a large project goes from 30s to 40s, and it's very clear that this comes from the locks because the involuntary context switches increase from 2'113'066 to 8'769'635, that's 4 times as many threads that block on each other. Fixing this requires changes in Salsa. Ideally, Salsa would shard locks similarly to how we handle interned structs. This should remove most congestion because checking a file is, most of the time, local to a single thread. But I don't think this is an entirely trivial change and @ibraheemdev is probably the best person to work on it. For now, I'll land the change that enables LRU for I still think it's important for us to enable LRU for semantic indexing because it's wasteful to keep all of them in memory when using workspace diagnostics. |
MichaReiser
changed the title
* origin/main: [ty] Improve `@override`, `@final` and Liskov checks in cases where there are multiple reachable definitions (#21767) [ty] Extend `invalid-explicit-override` to also cover properties decorated with `@override` that do not override anything (#21756) [ty] Enable LRU collection for parsed module (#21749) [ty] Support typevar-specialized dynamic types in generic type aliases (#21730) Add token based `parenthesized_ranges` implementation (#21738) [ty] Default-specialization of generic type aliases (#21765) [ty] Suppress false positives when `dataclasses.dataclass(...)(cls)` is called imperatively (#21729) [syntax-error] Default type parameter followed by non-default type parameter (#21657) new module for parsing ranged suppressions (#21441) [ty] `type[T]` is assignable to an inferable typevar (#21766) Fix syntax error false positives for `await` outside functions (#21763) [ty] Improve diagnostics for unsupported comparison operations (#21737) Move `Token`, `TokenKind` and `Tokens` to `ruff-python-ast` (#21760) [ty] Don't confuse multiple occurrences of `typing.Self` when binding bound methods (#21754) Use our org-wide Renovate preset (#21759) Delete `my-script.py` (#21751) [ty] Move `all_members`, and related types/routines, out of `ide_support.rs` (#21695)
* origin/main: [ty] Reachability constraints: minor documentation fixes (#21774) [ty] Fix non-determinism in `ConstraintSet.specialize_constrained` (#21744) [ty] Improve `@override`, `@final` and Liskov checks in cases where there are multiple reachable definitions (#21767) [ty] Extend `invalid-explicit-override` to also cover properties decorated with `@override` that do not override anything (#21756) [ty] Enable LRU collection for parsed module (#21749) [ty] Support typevar-specialized dynamic types in generic type aliases (#21730) Add token based `parenthesized_ranges` implementation (#21738) [ty] Default-specialization of generic type aliases (#21765) [ty] Suppress false positives when `dataclasses.dataclass(...)(cls)` is called imperatively (#21729) [syntax-error] Default type parameter followed by non-default type parameter (#21657)
6 participants
Summary
Enable LRU collection for the parsed module
and semantic indexso that ty only caches the results of a limited set of files, preventing ever-growing memory usage.Aggressively clear ASTs of third-party files during auto-import scanning.
The main downside of LRU collection is that we may end up doing more recomputations if we're too aggressive about removing ASTs and semantic indices. Ultimately, I think we have to experiment with and fine-tune this based on user feedback to find the right balance between compute/memory usage.
The other downside is that tracking the most recent values does add some overhead which shows up as a ~2% perf regression on the walltime benchmarks. I don't think there's much we can do about it other than, in the future, extend Salsa to make LRU a runtime feature that's only enabled in the LSP (although we have ideas to enable within-revision LRU which would also be useful for the CLI)
I also replaced the global tracker used in
heap_sizeto ensure subsequent calls tosalsa_memory_dumpreturn accurate results (e.g. source text suddenly reported that it has a size of 0 when the memory report was created more than once)Fixes astral-sh/ty#1707
Test Plan
Main, after start
Main, after auto completion
This PR, after startup
This PR, after auto complete
and parsed modules drops down to less than 200 after making a few more changes (LRU is deferred in that it requires a few changes before it's safe for salsa to delete it)
@ibraheemdev I'm not sure if the counts are correctly updated after LRU. Do we filter out memos without a value, or is it intentional that we don't, so that we still account for the metadata overhead?
Note: ty will still use more memory than reported by salsa because it currently doesn't release no-longer-used pages. But homeassistant now takes ~1.2 GB after LRU collection compared to 5.6 GB before, which is a substantial improvement