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 base-db to new Salsa #18305

Conversation

davidbarsky
Copy link
Contributor

@davidbarsky davidbarsky commented Oct 15, 2024

Trying out new Salsa in rust-analyzer by porting over base-db. I haven't fully ported it over, but given its interactions with the VFS/external files (and the plan outlined on Zulip)), I figured I'd post this for early review and feedback.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 15, 2024
@davidbarsky davidbarsky force-pushed the davidbarsky/port-basedb-to-new-salsa branch 3 times, most recently from 60a9bb1 to c5d6111 Compare October 16, 2024 19:04
@davidbarsky
Copy link
Contributor Author

Hmm, I'm not sure why the license check fails with

thread 'tidy::test' panicked at xtask/src/tidy.rs:182:9:
different set of licenses!
New Licenses:
  null

Missing Licenses:

Similarly, I don't know why the cross build is failing with:

error[E0463]: can't find crate for `salsa`
  --> crates/base-db/src/new_db.rs:11:5
   |
11 | use salsa::{Accumulator, Durability, Event};
   |     ^^^^^ can't find crate

I can't reproduce that issue locally.

@Veykril
Copy link
Member

Veykril commented Oct 17, 2024

The license check might be failing due to the git dependency (unsure how the logic there works in that check), the cross one failing is weird

@davidbarsky davidbarsky force-pushed the davidbarsky/port-basedb-to-new-salsa branch from c5d6111 to 294dd39 Compare October 17, 2024 21:43
@davidbarsky
Copy link
Contributor Author

For the license check, that should be fixed by salsa-rs/salsa#601.

@davidbarsky davidbarsky force-pushed the davidbarsky/port-basedb-to-new-salsa branch from 294dd39 to b963c53 Compare October 17, 2024 22:00
@ChayimFriedman2
Copy link
Contributor

Is there a summary of the differences between Salsa 2 and Salsa 3, for those of us who don't follow the news?

@davidbarsky
Copy link
Contributor Author

Is there a summary of the differences between Salsa 2 and Salsa 3, for those of us who don't follow the news?

Like, new Salsa and salsa-2022 or the new Salsa and the Salsa rust-analyzer uses?

@ChayimFriedman2
Copy link
Contributor

The Salsa rust-analyzer uses.

@davidbarsky
Copy link
Contributor Author

From a programming model standpoint, I wrote this document as a guide/reference. From a "what features are now possible" standpoint, new Salsa makes the following possible, or at the very least, a lot easier:

  1. Persistent/saved state of nameres. I want to try out persistent caching to make startup faster.
  2. Interned values will actually be cleaned up in new Salsa!
    a. New Salsa also gives out Copy values, not Clone, which pairs nicely with the new trait solver's requirements.
  3. Easy, pervasive parallelism. This would be a big deal for autocomplete!
  4. Salsa-native cycle recovery, which might make it possible to simplify some of the fixed point iterations that exist within rust-analyzer.

Not all the functionality above is currently implemented, but I feel comfortable betting that it will be shortly—a bunch of a folks are working on implementing cycle recovery, serialized state, and GC'ing interned values as I write this comment.

Anyways, porting ra over is a bit annoying, but... that's the job.

@Veykril
Copy link
Member

Veykril commented Oct 18, 2024

Also don't forget forking of the database, allowing speculative execution which simplifies completion context analysis (and also makes it more robust/less flaky)

@bors
Copy link
Contributor

bors commented Oct 21, 2024

☔ The latest upstream changes (presumably #17954) made this pull request unmergeable. Please resolve the merge conflicts.

@davidbarsky
Copy link
Contributor Author

I'm going to close this PR in favor of work I've been doing on a macro imitating the existing query_group macro: https://github.com/davidbarsky/db-ext-macro/. Few notes:

  1. That macro live in upstream Salsa; it's just easier to develop there because rust-analyzer reloads the workspace frequently. I'll open a PR against upstream Salsa by Monday.
  2. The macro mimics the original API query_group API pretty closely!
  3. I need to add support for cycle and interned annotations and figure out why the keys in an #[input]-annotated struct are being read as unused (correctly, I might add).

@davidbarsky davidbarsky closed this Nov 1, 2024
@davidbarsky davidbarsky deleted the davidbarsky/port-basedb-to-new-salsa branch November 8, 2024 18:40
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.

5 participants