perf: Non-Salsa-interned solver types - with GC for them #21295
Conversation
rustbot
added
the
S-waiting-on-review
ShoyuVanilla
left a comment
ShoyuVanilla
left a comment
There was a problem hiding this comment.
It's second time I'm looking into this code and it became a bit simpler as it has been split into some other relatively simple PRs 😄
Looks good to me 👍 Awesome work. I'm really waiting for this to be released.
|
Given the volume and unsafety of the changes, I'd like a second approving eye before merging this. |
That means stop using Salsa for interning solver types.
A GC is triggered every X revisions, and is synchronous, unfortunately.
Veykril
left a comment
Veykril
left a comment
There was a problem hiding this comment.
Still reviewing but I'd love docs for the intern crate stuff
| /// which will usually end up causing a bunch of incorrect diagnostics on startup. | ||
| pub(crate) incomplete_crate_graph: bool, | ||
|
|
||
| pub(crate) revisions_until_next_gc: usize, |
There was a problem hiding this comment.
Follow up thing I'd love is if we could queue up clears on a time basis as well, like notice no (lsp) inputs where given in the last 10 seconds or so, and then hit it, to reduce the pause times we might get from user inputs
There was a problem hiding this comment.
That'll be hard to do given that GC is blocking and other operations accessing the database cannot run in parallel to it. And I'm not sure that's needed either; on omicron, a repo with lots of types (far more than r-a, for instance), at the end of analysis-stats, involving a lot more types than regular IDE experience (inferring all bodies), GC completed in under a second on my machine.
Anyway I don't want to do this in this PR.
There was a problem hiding this comment.
Definitely not for this PR to make this clear, and this is also something ive wanted to do for general salsa LRU eviction. This completing in less than a second is the wrong metric though, the important bit is that this should on average (in an ideal world) complete in 10ms or less, otherwise this blocks typing noticably.
There was a problem hiding this comment.
I know, that's why I emphasized that this is a lot more types than usual. FWIW I tested r-a in the editor with GC set for every revision, and it was snappy.
|
Sorry for the split reviews, not a fan of githubs review experience here at all ... Anyways I'd definitely love more documentation on the underlying machinery, its a complex unsafe system so docs are really needed here |
|
Yeah, I agree we need more docs on this. |
ChayimFriedman2
force-pushed
the
non-salsa-interneds-v3
branch
from
8f294a1 to
36c9f62
Compare
|
@Veykril I added a third commit with an extensive documentation for the unsafe stuff. |
ChayimFriedman2
force-pushed
the
non-salsa-interneds-v3
branch
from
63bb060 to
5e2dfbb
Compare
| //! Making mistakes is hard due to GC [`InternedRef`] wrappers not implementing `salsa::Update`, meaning | ||
| //! Salsa will ensure you do not store them in queries or Salsa-interneds. However it's still *possible* | ||
| //! without unsafe code (for example, by storing them in a `static`), which is why triggering GC is unsafe. |
There was a problem hiding this comment.
If the lifetime of the InternedRef was an actually used lifetime then the static (and by extension thread locals) issue wouldn't be a problem I guess
4 participants
This saves an enormous amount of time and memory, as described in #t-compiler/rust-analyzer > Non-copy types for the solver.