rust-analyzer icon indicating copy to clipboard operation
rust-analyzer copied to clipboard

Improve `Semantics` construction performance

Open Veykril opened this issue 1 year ago • 2 comments

hir::Semantics is a general cache used to be able to resolve any syntax node to its semantic definition. This is generally slow given the kind of work it does, but quite often we do a lot more work than necessary, populating unnecessary DynMap tables and more importantly, we populate it via parsing, where the parsing happens through direct database calls, even though the Semantics itself caches these. That means for big enough files we can end up with thrashing the LRU cache a fair bit. We should fix that, a related problem is the HasSource trait going through the database instead of Semantics which means we can end up with nodes not cached by Semantics ultimately ending up in panics when it tries to lookup stuff (which has happened in the past).

So the things that need to be changed are:

  • Change the signature of (or add a second version) HasSource::source to take a reference to the Semantics instead that does the right thing https://github.com/rust-lang/rust-analyzer/blob/062e1b9b81542680afde1aa47fa5316ac5133461/crates/hir/src/has_source.rs#L29
  • Change https://github.com/rust-lang/rust-analyzer/blob/062e1b9b81542680afde1aa47fa5316ac5133461/crates/hir-def/src/src.rs#L14-L21 to support this
  • Change https://github.com/rust-lang/rust-analyzer/blob/062e1b9b81542680afde1aa47fa5316ac5133461/crates/hir-def/src/child_by_source.rs#L26-L33 to support this

This will most likely require adding callbacks to some of the hir-def crate APIs to abstract away the parsing (and then add / keep the current APIs as convenience APIs where Semantics won't be involved).

Veykril avatar Apr 21 '24 06:04 Veykril

Mostly unrelated, but in the past we used to construct multiple Semantics during the execution of a single request. It might be worth checking whether that's still the case.

lnicola avatar Apr 22 '24 07:04 lnicola

@rustbot claim

Kohei316 avatar Jun 18 '24 11:06 Kohei316

Semantics caches those things but the direction is opposite; the cache direction is Source -> Def, but the functions we need to modify have the signatures Def -> Source. Should this be done by making the caches bidirectional or doing some inefficient inverse search like HashMap::iter().find()?

edit) The former seems to contradict #17380 🤔

ShoyuVanilla avatar Jan 21 '25 10:01 ShoyuVanilla

Hey @Veykril , is this up for grab?

Shourya742 avatar Feb 27 '25 03:02 Shourya742

@rustbot claim

Shourya742 avatar Mar 04 '25 07:03 Shourya742

Semantics caches those things but the direction is opposite; the cache direction is Source -> Def, but the functions we need to modify have the signatures Def -> Source. Should this be done by making the caches bidirectional or doing some inefficient inverse search like HashMap::iter().find()?

edit) The former seems to contradict #17380 🤔

Good question, LRU cache invalidation wont be an issue with the new salsa anymore as the LRU doesnt trigger mid query computations. It has been a while since ive though about this issue

Veykril avatar Mar 04 '25 09:03 Veykril