Skip to content
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

Conversation

davidbarsky
Copy link
Contributor

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 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:

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:

  1. 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.
  2. 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.
  3. 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.
  4. 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.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 17, 2025
@davidbarsky 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
Copy link
Contributor Author

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.

@davidbarsky 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
@the8472
Copy link
Member

the8472 commented Jan 25, 2025

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
Copy link
Member

Veykril commented Jan 25, 2025

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

Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(partial review)

Comment on lines -4 to -6
#[cfg(feature = "ra-salsa")]
use ra_salsa::InternId;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@davidbarsky
Copy link
Contributor Author

@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 davidbarsky force-pushed the davidbarsky/port-rust-analyzer-to-new-salsa branch from 54ef4ea to 83fe206 Compare February 5, 2025 18:45
@davidbarsky
Copy link
Contributor Author

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
Copy link
Member

Veykril commented Feb 5, 2025

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
Copy link
Contributor Author

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 queries1, 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.

Footnotes

  1. 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.

@davidbarsky davidbarsky force-pushed the davidbarsky/port-rust-analyzer-to-new-salsa branch from 83fe206 to 5b4a994 Compare February 20, 2025 15:44
@Veykril Veykril mentioned this pull request Feb 26, 2025
Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another partial review, still need to go through the ide crates and the main rust-analyzer crate

@Veykril
Copy link
Member

Veykril commented Feb 26, 2025

Also feel free to squash the history here to make your rebase life easier, I don't really care for my commit(s) in there

Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I've done a full pass now

@davidbarsky davidbarsky force-pushed the davidbarsky/port-rust-analyzer-to-new-salsa branch from 5b4a994 to 96c645e Compare February 27, 2025 22:39
@davidbarsky davidbarsky force-pushed the davidbarsky/port-rust-analyzer-to-new-salsa branch 2 times, most recently from dea1fe7 to 65d45e3 Compare March 3, 2025 17:36
Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Judging by the ide test failure, the weird editioned file handling is likely the cause (we are scrambling the editions around somewhere)

@davidbarsky
Copy link
Contributor Author

davidbarsky commented Mar 4, 2025

Judging by the ide test failure, the weird editioned file handling is likely the cause (we are scrambling the editions around somewhere)

the keyword highlighting one? Yeah, good point. Will dig around, must be something kinda inadvertently silly.

I'll fix the weird construction/deconstruction of EditionedFileId, but it happened due it being detritus from repeated refactors and a big of lingering brainfog from being sick.

@davidbarsky davidbarsky force-pushed the davidbarsky/port-rust-analyzer-to-new-salsa branch 3 times, most recently from 47fda29 to 57353c7 Compare March 5, 2025 16:06
@davidbarsky
Copy link
Contributor Author

davidbarsky commented Mar 5, 2025

Updates:

  1. Fixed the weird File ID refactoring detritus stuff that Lukas pointed out.
  2. The cross-compilation tests are failing on powerpc-unknown-linux-gnu, as new Salsa is directly using a AtomicU64 to reduce memory usage. We'll probably cfg out the 64-bit specific optimizations on 32-bit targets, as discussed on the Salsa Zulip, but if it's too annoying, we'll use https://github.com/taiki-e/portable-atomic in Salsa instead.
  3. The syntax highlighting test failures (syntax_highlighting::tests::{test_keyword_highlighting, test_keyword_macro_edition_highlighting, test_rainbow_highlighting}) are failing. I've just pushed a commit that include the "actual" changes suffixed by wat. I'm pretty sure it's related to me messing up default editions somewhere, but maybe based on the nature of the failures, but I'm not entirely sure where. Maybe someone will know better than I do! I'll post another update if/when I find the cause. Note that I'll remove this commit before landing this PR.
  4. This is no longer marked as a draft!

We'll still need to:

  1. Vendor https://github.com/davidbarsky/db-ext-macro/ into rust-analyzer. I'll actually do that today.
  2. Cut a Salsa 1.0-alpha-1 release to unblock @carljm from landing fixed-point iteration support salsa-rs/salsa#603.

@davidbarsky davidbarsky marked this pull request as ready for review March 5, 2025 18:12
@davidbarsky davidbarsky force-pushed the davidbarsky/port-rust-analyzer-to-new-salsa branch from 1cf3bdb to b8a6a3d Compare March 5, 2025 18:19
Comment on lines 236 to 238
// FIXME: is this right?
if self.is_root() {
return Edition::LATEST;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be the cause for the syntax highlighting test failures

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// FIXME: is this right?
if self.is_root() {
return Edition::LATEST;
}
if self.is_root() {
return Edition::from_u32(self.0.as_u32() - (SyntaxContext::MAX_ID - Edition::LATEST as u32));
}

Note Edition::from_u32 isn't a thing, gotta create that as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Applying both makes test_keyword_macro_edition_highlighting pass, but test_keyword_highlighting and test_rainbow_highlighting still fail. Not sure why, but working on vendoring the query group macro (and updating it to the latest Salsa...).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rainbow i think isnt a problem, we likely are hashing different data now, as long as the hashes are stable and equal as before (wrt to positions they appear in). The other keyword one is still edition related though hmm

Copy link
Member

@Veykril Veykril Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it is weird that the hashes change, they shouldn't nothing there goes through salsa ... Ah nvm, rustc-hash got bumped in this PR, that's why they changed I imagine

@davidbarsky davidbarsky force-pushed the davidbarsky/port-rust-analyzer-to-new-salsa branch from b8a6a3d to 8ae7f64 Compare March 6, 2025 14:04
@davidbarsky davidbarsky force-pushed the davidbarsky/port-rust-analyzer-to-new-salsa branch from 8ae7f64 to 43ec93c Compare March 10, 2025 15:24
@davidbarsky
Copy link
Contributor Author

Rebased + tests should be fixed. Please review the test changes in handlers::convert_bool_to_enum::tests::field_enum_cross_file and render::tests::score_fn_type_and_name_match! I assume the test failures are due to some edition-related stuff, but if those changes are good, I'll squash all the commits into one and merge this.

@Veykril
Copy link
Member

Veykril commented Mar 10, 2025

handlers::convert_bool_to_enum::tests::field_enum_cross_file might be due to Ord changes, render::tests::score_fn_type_and_name_match definitely due to Ord changes. Neither are problematic right now

@davidbarsky
Copy link
Contributor Author

okay! I will squash and add this to the merge queue (!)

@davidbarsky davidbarsky force-pushed the davidbarsky/port-rust-analyzer-to-new-salsa branch from 43ec93c to f956b77 Compare March 10, 2025 15:43
@davidbarsky davidbarsky force-pushed the davidbarsky/port-rust-analyzer-to-new-salsa branch from f956b77 to a68aa7d Compare March 10, 2025 17:09
@davidbarsky davidbarsky force-pushed the davidbarsky/port-rust-analyzer-to-new-salsa branch from a68aa7d to 74620e6 Compare March 10, 2025 17:31
@davidbarsky davidbarsky added this pull request to the merge queue Mar 10, 2025
Merged via the queue into rust-lang:master with commit 44f18c3 Mar 10, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants