internal: port rust-analyzer to new Salsa #18964

davidbarsky · 2025-01-17T19:06:42Z

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:

With @nikomatsakis's help, I created 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.
@Veykril ported crates/span to new Salsa, which makes use of self-referential interning. This is needed to support macro hygiene.
New Salsa's structs typically have a '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:

@ShoyuVanilla reduced rust-analyzer's memory usage on analysis-stats from 8 gigabytes to 4 gigabytes by through https://lgithub.com/salsa-rs/salsa/pull/614/.
@ChayimFriedman2 and @Veykril made further improvements to Salsa's memory usage, which reduced usage by around 575 megabytes.
Chayim implemented #[salsa::enum], which reduced memory usage by about 150 megabytes and made rust-analyzer's usage of new Salsa more idiomatic.
I reenabled LRU, which reduced memory usage by an additional gigabyte.

Performance

On rust-analyzer analysis-stats, new Salsa is slightly slower and uses slightly more memory.
- On bfb8127, new Salsa uses 2201mb of memory, whereas old Salsa uses ~2000 megabytes.
On 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:

Reintroduce debug item printing. However, this is blocked on feature: Add support for dumping database contents salsa-rs/salsa#640 and some additional changes inside of rust-analyzer, but they're not complicated changes.
Figure out why the changes in the 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.
Release db-ext-macro.
1. I'd like to rename db-ext-macro to something else, or have it live in Salsa. I'm unsure which option I'd prefer right now.
Release the new Salsa to crates.io.

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.

davidbarsky · 2025-01-20T23:05:12Z

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:

My macro also implements traits like Eq and Hash. And it implements them by hashing the ID only, because I thought it will be more efficient.
But have we global, unrelated to salsa, interning. For example, that's how we intern Tys. And this global interning will persist even after we switch dbs. But in different dbs, different IDs can map to different kinds!
So we might switch the type of an AdtId, for example, because we mapped it to the incorrect interned key!

As a short-term fix, he's required interned values to implement PartialEq and friends. Longer-term, we'll move this interner into new Salsa.

the8472 · 2025-01-25T00:05:51Z

reduced rust-analyzer's memory usage on analysis-stats from 8 gigabytes to 4 gigabytes by through https://lgithub.com/salsa-rs/salsa/pull/614/.

contains a typo in the link

Veykril · 2025-01-25T09:49:19Z

Regarding db-ext-macro's home, we could also consider placing it in-tree here. That might be easiest tbh.

Veykril

(partial review)

Veykril · 2025-02-05T09:02:19Z

Cargo.toml

@@ -11,7 +11,7 @@ authors = ["rust-analyzer team"]
 repository = "https://github.com/rust-lang/rust-analyzer"

 [profile.dev]
-debug = 1
+debug = 2


Suggested change

debug = 2

debug = 1

(reminder)

Veykril · 2025-02-05T09:02:42Z

Cargo.toml

@@ -27,7 +27,7 @@ miniz_oxide.opt-level = 3
 [profile.release]
 incremental = true
 # Set this to 1 or 2 to get more useful backtraces in debugger.
-debug = 0
+debug = 2


Suggested change

debug = 2

debug = 0

(reminder)
also for this you could use the dev-rel profile

Veykril · 2025-02-05T09:12:15Z

crates/base-db/src/input.rs

how come that there are formatting changes in here (without CI ever complaining about them) 🤨

Veykril · 2025-02-05T09:14:33Z

crates/profile/src/lib.rs

-    #[cfg(not(feature = "cpu_profiler"))]
-    #[allow(clippy::print_stderr)]
-    {
-        eprintln!(
-            r#"cpu profiling is disabled, uncomment `default = [ "cpu_profiler" ]` in Cargo.toml to enable."#
-        );
-    }
+    // #[cfg(not(feature = "cpu_profiler"))]
+    // #[allow(clippy::print_stderr)]
+    // {
+    //     eprintln!(
+    //         r#"cpu profiling is disabled, uncomment `default = [ "cpu_profiler" ]` in Cargo.toml to enable."#
+    //     );
+    // }


Whats up with this

Veykril · 2025-02-05T09:17:53Z

crates/span/src/lib.rs

-#[cfg(feature = "ra-salsa")]
-use ra_salsa::InternId;
-


Can we re-introduce this cfg'ing for the new salsa? The point of that is that the proc-macro server has no need to depend on salsa

Veykril · 2025-02-05T09:25:13Z

crates/rust-analyzer/src/cli/rustc_tests.rs

Whats up with the changes here?

Veykril · 2025-02-05T09:27:40Z

crates/base-db/src/lib.rs

+
+#[salsa::interned(no_lifetime)]
+pub struct EditionedFileId {
+    pub file_id: FileText,


Suggested change

pub file_id: FileText,

pub file_id: vfs::FileId,

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`

…tests.rs`

davidbarsky · 2025-02-05T17:50:41Z

@Veykril Regarding crates/rust-analyzer/src/cli/rustc_tests.rs, it looks like some parts of new Salsa—namely, (dyn salsa::accumulator::accumulated::AnyAccumulated + 'static)—don't implement UnwindSafe. Here's the error:

error[E0277]: the type `(dyn salsa::accumulator::accumulated::AnyAccumulated + 'static)` may not be safely transferred across an unwind boundary
    --> crates/rust-analyzer/src/cli/rustc_tests.rs:163:60
     |
163  |                           let res = std::panic::catch_unwind(move || {
     |  ___________________________________------------------------_^
     | |                                   |
     | |                                   required by a bound introduced by this call
164  | |                             analysis.full_diagnostics(
165  | |                                 diagnostic_config,
166  | |                                 ide::AssistResolveStrategy::None,
...    |
169  | |                         });
     | |_________________________^ `(dyn salsa::accumulator::accumulated::AnyAccumulated + 'static)` may not be safely transferred across an unwind boundary
     |
     = help: the trait `UnwindSafe` is not implemented for `(dyn salsa::accumulator::accumulated::AnyAccumulated + 'static)`
     = note: required for `std::ptr::Unique<(dyn salsa::accumulator::accumulated::AnyAccumulated + 'static)>` to implement `UnwindSafe`
note: required because it appears within the type `std::boxed::Box<(dyn salsa::accumulator::accumulated::AnyAccumulated + 'static)>`
    --> /Users/dbarsky/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/boxed.rs:231:12
     |
231  | pub struct Box<
     |            ^^^
     = note: required for `HashMap<IngredientIndex, std::boxed::Box<(dyn salsa::accumulator::accumulated::AnyAccumulated + 'static)>, FxBuildHasher>` to implement `UnwindSafe`
note: required because it appears within the type `salsa::accumulator::accumulated_map::AccumulatedMap`
    --> /Users/dbarsky/.cargo/git/checkouts/salsa-deab236162935b01/592552b/src/accumulator/accumulated_map.rs:10:12
     |
10   | pub struct AccumulatedMap {
     |            ^^^^^^^^^^^^^^
note: required because it appears within the type `salsa::active_query::ActiveQuery`
    --> /Users/dbarsky/.cargo/git/checkouts/salsa-deab236162935b01/592552b/src/active_query.rs:17:19
     |
17   | pub(crate) struct ActiveQuery {
     |                   ^^^^^^^^^^^
note: required because it appears within the type `std::marker::PhantomData<salsa::active_query::ActiveQuery>`
    --> /Users/dbarsky/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/marker.rs:757:12
     |
757  | pub struct PhantomData<T: ?Sized>;
     |            ^^^^^^^^^^^
note: required because it appears within the type `alloc::raw_vec::RawVec<salsa::active_query::ActiveQuery>`
    --> /Users/dbarsky/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/raw_vec.rs:70:19
     |
70   | pub(crate) struct RawVec<T, A: Allocator = Global> {
     |                   ^^^^^^
note: required because it appears within the type `Vec<salsa::active_query::ActiveQuery>`
    --> /Users/dbarsky/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/vec/mod.rs:397:12
     |
397  | pub struct Vec<T, #[unstable(feature = "allocator_api", issue = "32838")] A: Allocator = Global> {
     |            ^^^
note: required because it appears within the type `std::cell::UnsafeCell<Vec<salsa::active_query::ActiveQuery>>`
    --> /Users/dbarsky/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/cell.rs:2076:12
     |
2076 | pub struct UnsafeCell<T: ?Sized> {
     |            ^^^^^^^^^^
note: required because it appears within the type `RefCell<Vec<salsa::active_query::ActiveQuery>>`
    --> /Users/dbarsky/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/cell.rs:730:12
     |
730  | pub struct RefCell<T: ?Sized> {
     |            ^^^^^^^
note: required because it appears within the type `ZalsaLocal`
    --> /Users/dbarsky/.cargo/git/checkouts/salsa-deab236162935b01/592552b/src/zalsa_local.rs:30:12
     |
30   | pub struct ZalsaLocal {
     |            ^^^^^^^^^^
note: required because it appears within the type `Storage<RootDatabase>`
    --> /Users/dbarsky/.cargo/git/checkouts/salsa-deab236162935b01/592552b/src/storage.rs:25:12
     |
25   | pub struct Storage<Db: Database> {
     |            ^^^^^^^
note: required because it appears within the type `std::mem::ManuallyDrop<Storage<RootDatabase>>`
    --> /Users/dbarsky/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/mem/manually_drop.rs:157:12
     |
157  | pub struct ManuallyDrop<T: ?Sized> {
     |            ^^^^^^^^^^^^
note: required because it appears within the type `RootDatabase`
    --> /Users/dbarsky/Developer/rust-analyzer/crates/ide-db/src/lib.rs:79:12
     |
79   | pub struct RootDatabase {
     |            ^^^^^^^^^^^^
note: required because it appears within the type `Analysis`
    --> /Users/dbarsky/Developer/rust-analyzer/crates/ide/src/lib.rs:219:12
     |
219  | pub struct Analysis {
     |            ^^^^^^^^
note: required because it's used within this closure
    --> crates/rust-analyzer/src/cli/rustc_tests.rs:163:60
     |
163  |                         let res = std::panic::catch_unwind(move || {
     |                                                            ^^^^^^^
note: required by a bound in `std::panic::catch_unwind`
    --> /Users/dbarsky/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/panic.rs:357:40
     |
357  | pub fn catch_unwind<F: FnOnce() -> R + UnwindSafe, R>(f: F) -> Result<R> {
     |                                        ^^^^^^^^^^ required by this bound in `catch_unwind`

I think that #19093 is also slightly problematic from a "get rust-analyzer to compile" standpoint, but I don't know enough about unwind safety to say whether I should wrap analysis— and therefore, accumulators—in a AssertUnwindSafe as the documentation points out. Put differently, if rust-analyzer was to make more pervasive use of accumulators, would we risk getting incorrect results, or...?

davidbarsky · 2025-02-05T18:47:08Z

I decided to wrap some Analysis calls in AssertUnwindSafe, as we're not really using accumulators in rust-analyzer. Once we do, I think it might be a good idea to revisit that usage.

Veykril · 2025-02-05T19:51:28Z

That's fine, the problematic type only appears within ActiveQuery, the query stack of the runtime which is always empty at this point. So AssertUnwindSafe is okay to be used here

davidbarsky · 2025-02-06T22:40:54Z

Alright, few updates:

Debug Printing

I put up salsa-rs/salsa#676, which makes it possible for us to access the underlying data without involving a salsa::Database's storage (which may not be accessible!). I've used this to port over dump_syntax_contexts in crates/hir-expand/src/hygiene.rs locally, but I haven't pushed yet—I will, though.

Unfortunately, the approach is not applicable to debug printing in crates/ide/src/status.rs, as we'd need to access a tracked function's memos and it's not immediately obvious to me how to do that. Given that it's a single-digit number of queries¹, I think I'd prefer to accept a regression in the quality of debug output and prioritize porting those queries over to idiomatic Salsa (c.f., #19098), especially since we've blocked landing any database changes until this PR lands.

Enums

I've also opened salsa-rs/salsa#677, which mostly just publishes @ChayimFriedman2's #[salsa::enum]/#[salsa::supertype] work.

The queries in question are SourceDatabase::file_text, RootQueryDb::parse, ExpandDatabase::parse_macro_expansion, SymbolsDatabase::module_symbols, SymbolsDatabase::library_symbols, ExpandDatabase::ast_id_map, and DefDatabase::block_def_map. ↩

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 17, 2025

davidbarsky force-pushed the davidbarsky/port-rust-analyzer-to-new-salsa branch 2 times, most recently from e2767ec to 3a7fd79 Compare January 20, 2025 22:54

davidbarsky mentioned this pull request Jan 21, 2025

Lazy Workspace/Project Discovery #17537

Open

davidbarsky force-pushed the davidbarsky/port-rust-analyzer-to-new-salsa branch 2 times, most recently from c85177e to c762e4e Compare January 24, 2025 21:19

davidbarsky mentioned this pull request Jan 28, 2025

internal: backout hir-* changes from #19001 #19063

Merged

davidbarsky force-pushed the davidbarsky/port-rust-analyzer-to-new-salsa branch from c762e4e to 54ef4ea Compare January 28, 2025 19:16

alanz mentioned this pull request Feb 3, 2025

Move salsa cancellation into Drop for Server WhatsApp/erlang-language-platform#74

Closed

Veykril mentioned this pull request Feb 5, 2025

Idiomatic Salsa Tracking Issue #19098

Open

35 tasks

Veykril reviewed Feb 5, 2025

View reviewed changes

davidbarsky and others added 16 commits February 5, 2025 11:36

internal: port basedb to new salsa

a4f296c

internal: point salsa against personal fork

b78c3d6

span: qualify ra_salsa imports

2f448b8

internal: port hir-expand to new salsa

221ee61

internal: port hir-def to new salsa

f28b602

internal: ensure new salsa works in ra

502ea23

internal: reintroduce file_source_root

7f1eb84

internal: update test snapshots, remove dbg!, fix warnings

7afdfcf

internal: mutate file inputs; allowing editing

28389a0

internal: fix conflicts/issues; deduplicate some SourceRootDb queries

df877dd

fix: ensure that all SourceDatabase implementations mutate inputs

026d777

use Database::unwind_if_revision_cancelled

9f590d1

support per-edition tokens

f2ae1c9

cleanup SyntaxContext

57f4303

Intern FileId/EditionedFileId into a Salsa struct

c152a4c

davidbarsky and others added 8 commits February 5, 2025 12:34

update salsa fork

cd8fa11

tune LRU capacities

d7d6422

enums

7fc7b94

fix cargo.toml stuff

4b17f69

fix warnings

d8805a6

allow unicode licenses

de2478a

update rust-analyzer to new interned struct

db4d4c0

chore: undo unwinding changes in `crates/rust-analyzer/src/cli/rustc_…

1ddbf1e

…tests.rs`

chore: use more AssertUnwindSafe (unwound accumulators can't hurt us)

83fe206

davidbarsky force-pushed the davidbarsky/port-rust-analyzer-to-new-salsa branch from 54ef4ea to 83fe206 Compare February 5, 2025 18:45

Veykril mentioned this pull request Feb 6, 2025

Introduce RefUnwindSafe StorageHandle salsa-rs/salsa#675

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal: port rust-analyzer to new Salsa #18964

internal: port rust-analyzer to new Salsa #18964

davidbarsky commented Jan 17, 2025

davidbarsky commented Jan 20, 2025

the8472 commented Jan 25, 2025

Veykril commented Jan 25, 2025

Veykril left a comment

Veykril Feb 5, 2025

Veykril Feb 5, 2025

Veykril Feb 5, 2025

Veykril Feb 5, 2025

Veykril Feb 5, 2025

Veykril Feb 5, 2025

Veykril Feb 5, 2025

davidbarsky commented Feb 5, 2025

davidbarsky commented Feb 5, 2025

Veykril commented Feb 5, 2025

davidbarsky commented Feb 6, 2025

internal: port rust-analyzer to new Salsa #18964

Are you sure you want to change the base?

internal: port rust-analyzer to new Salsa #18964

Conversation

davidbarsky commented Jan 17, 2025

Migration Approach

Performance

Things to Fix Before to Merging

Acknowledgements

davidbarsky commented Jan 20, 2025

the8472 commented Jan 25, 2025

Veykril commented Jan 25, 2025

Veykril left a comment

Choose a reason for hiding this comment

Veykril Feb 5, 2025

Choose a reason for hiding this comment

Veykril Feb 5, 2025

Choose a reason for hiding this comment

Veykril Feb 5, 2025

Choose a reason for hiding this comment

Veykril Feb 5, 2025

Choose a reason for hiding this comment

Veykril Feb 5, 2025

Choose a reason for hiding this comment

Veykril Feb 5, 2025

Choose a reason for hiding this comment

Veykril Feb 5, 2025

Choose a reason for hiding this comment

davidbarsky commented Feb 5, 2025

davidbarsky commented Feb 5, 2025

Veykril commented Feb 5, 2025

davidbarsky commented Feb 6, 2025

Debug Printing

Enums

Footnotes