-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
internal: port rust-analyzer to new Salsa #18964
base: master
Are you sure you want to change the base?
internal: port rust-analyzer to new Salsa #18964
Conversation
internal: remove chalk bump
This commit consists of several changes: - Reimplement `SyntaxContext` using new salsa's self-referential interning - fix `SyntaxContext`'s `StructData` Hash and PartialEq impls - fix `SyntaxContext` Debug impl - Do not store IDs of inputs in the inputs themselves - the removal of `ra-salsa`
aa6bfee
to
e2767ec
Compare
e2767ec
to
3a7fd79
Compare
Chayim fixed the test failures: this was due us mixing and matching Salsa interning with rust-analyzer's own, home-grown interning. He put it succinctly here:
As a short-term fix, he's required interned values to implement |
This PR ports rust-analyzer to the new Salsa. It is big, partially incomplete, and best reviewed commit-by-commit. I will be rebasing often.
(Why new Salsa? It unblocks a lot of work described in #17491.)
Migration Approach
To get rust-analyzer compiling and passing tests, this PR did the following:
db-ext-macro
to mimic rust-analyzer's usage of old Salsa's query groups. Over time, rust-analyzer will migrate to new Salsa's idioms, moving away from the query group pattern. For additional details on the differences between old Salsa and new Salsa, please refer to this document.crates/span
to new Salsa, which makes use of self-referential interning. This is needed to support macro hygiene.'db
lifetime. However, the presence of the'db
lifetime made migrating a large number of interned structs challenging, hence feature: add#[salsa::interned_sans_lifetime]
macro salsa-rs/salsa#632.With these changes, all tests were passing (no correctness issues!),
analysis-stats
produced the same results as rust-analyzer on the old Salsa, and—critically—the new Salsa-powered rust-analyzer was usable as my daily driver. Unfortunately, memory usage was too high. I'm really grateful to the following folks who helped fix that regression:analysis-stats
from 8 gigabytes to 4 gigabytes by through https://lgithub.com/salsa-rs/salsa/pull/614/.#[salsa::enum]
, which reduced memory usage by about 150 megabytes and made rust-analyzer's usage of new Salsa more idiomatic.Performance
rust-analyzer analysis-stats
, new Salsa is slightly slower and uses slightly more memory.crates/rust-analyzer/src/integrated_benchmarks.rs
, new Salsa is slightly faster on both cold and warm updates. These benchmarks should probably be more rigorous, but new Salsa was consistently faster.Things to Fix Before to Merging
There's a few things to do before this change is merged:
enum
/#[salsa::supertype]
commit cause tests to fail to type confusion in the underlying Salsa pages. Chayim is working on root-causing this, but if he's unable to determine the cause, I can make the enum changes a separate pull request.db-ext-macro
.db-ext-macro
to something else, or have it live in Salsa. I'm unsure which option I'd prefer right now.Acknowledgements
This wouldn't have been possible without the assistance of many folks including @ChayimFriedman2, @ShoyuVanilla, @nikomatsakis, and @Veykril. I'm sure I'm forgetting some people, to whom I apologize.