perf: Cache trait solving across queries in the same revision

[ChayimFriedman2]

Caching trait solving can do a lot to speed. Unfortunately it also consume a huge amount of memory. Therefore, as part of the migration to the new solver Jack Huey disabled caching of trait solving (he made the query transparent).

The PR proposes a middle ground: do cache trait solving, but only for the same revision. This allows us to be safe because during a revision the inputs cannot change.

The result is hopefully much better performance to features that tend to do a bulk of trait solving, and also repeat the same query (e.g. inference then IDE features).

There is another limitation: results are only cached in the same thread, to remove the need for synchronization which will be expensive. More measurements are required to check whether it's better to use a synchronized global cache, or maybe stay with a thread-local cache but batch multiple feature requests (highlighting, inlay hints etc.) of the same file to the same thread.

Alongside the actual cache we store the revision, because we need to verify it (we can't eagerly clear caches when incrementing the revision), and also the address of the db to prevent multiple dbs from interleaving (this is mostly relevant in tests, although injected highlighting also uses a new db, therefore maybe it's better to move it to a separate thread).

This "games" analysis-stats to both be way faster and use way more memory; the former is because analysis-stats doesn't increment revisions, therefore all queries share the cache and hit ratio is way too good, the latter is because analysis-stats doesn't increment revisions and therefore the cache isn't cleared. Both are not representative of a typical IDE scenario.

[ShoyuVanilla]

Again, I'm +1 on this as this would improve performance without too much regression on memory spikes in typical usecase scenarios unless opening all the files across the project without editing anything

[ChayimFriedman2]

The test failure is because different DBs get allocated the exact same address...

I wonder what's best to solve this. Salsa uses a per-db nonce but it's not exposed. We could make our own but it's not nice...

[ChayimFriedman2]

I went ahead and solved it by making our own nonce. Not pretty, but works.

[ChayimFriedman2]

@rust-lang/rust-analyzer what is your opinion on this? @ShoyuVanilla already said they want this.

[ShoyuVanilla]

Yes, I've tried working with next-gen solver r-a and it's quite unworkable due to so much hangs. Though I haven't tried this PR's branch yet - will do tomorrow 😅 - but I strongly agree with that we need some action like this.

Edit) I think the best is to find out what's going on borrowck like in this comment #20503 (comment)
but this resolves the hangs according to #20503 (comment) and in general, I'm inclined to caching things as I personally have more RAM than CPU 😅 Of course, I think we must inspect borrowck and cut down memory usages by a lot as well, but I think this is a good middle ground anyway.

[ChayimFriedman2]

@rust-lang/rust-analyzer This is a second call. If I don't hear any objections, I'll merge this based on the approval from @ShoyuVanilla and because this renders r-a essentially unusable.

[davidbarsky]

I'm okay with doing this for now; we can revisit other caching strategies later on, need be.

[ChayimFriedman2]

@davidbarsky FWIW, I don't think we could find a fundamentally better caching strategy. I see some possible strategies:

1. We make trait_solve() a query like before. This, in some sense, even less performant than the approach in this PR (Salsa overhead+not caching queries the solver internally does, only what we externally ask it) - plus it consumes a huge amount of memory.
2. We cache trait solving, but don't do it via Salsa but rather via a customized system. This will be harder to create and maintain, still consume a lot of memory (although probably less than Salsa), still not cache inner solver queries and still be less performant due to the need to verify the output being up-to-date.
3. We somehow integrate with the new solver's cache to also cache inner queries across revisions. This will require approval from T-types, plus some of the above drawbacks.
4. We don't cache anything. This gives little benefit over this PR, as memory with this PR should be unimportant on continuous usage and speed will suffer a lot (to the point of being unusable, as we saw).
5. We do this PR.

However, I do think it's possible we can optimize more within this strategy, for example by batching requests to the same file to the same thread as I said above.

[lnicola]

This feels like a band-aid (especially the thread-local caches), but I think we need.

We'll probably have to find a better way to evaluate the memory usage, though.

[ChayimFriedman2]

This feels like a band-aid (especially the thread-local caches), but I think we need.

I agree it's kinda band-aid, but it's not that bad (just a few loc) and the gains are huge.

We'll probably have to find a better way to evaluate the memory usage, though.

We can clear the solver cache before measuring the memory, just like we bump the revision to trigger LRU.

[ShoyuVanilla]

Since there are no objections, I’ll go ahead and merge this. Given that we’re pausing stable releases, I think we should try out many experiments to improve rust-analyzer before we resume.