Skip to content

Bump salsa #19923

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Bump salsa #19923

wants to merge 1 commit into from

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented Jun 4, 2025

Just a quick experiment, this is using some of the things incorrectly. Mainly concerned with the memory impact of some upstream changes (ID has been changed from u32 to u64 which I believe has big implications)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 4, 2025
@Veykril Veykril marked this pull request as draft June 4, 2025 11:39
@Veykril
Copy link
Member Author

Veykril commented Jun 4, 2025

Oh ... we can't actually bump this because it would need SyntaxContext to change to a u64 I think which is a breaking change for the proc-macro RPC API

@ChayimFriedman2
Copy link
Contributor

Yeah there is also a problem with placeholders which are u32 but need to be transmuted into salsa::Id.

@Veykril
Copy link
Member Author

Veykril commented Jun 5, 2025

Actual, syntax context isnt really an issue, as we can (and already are) fall back to the TokenId server model.

@ChayimFriedman2
Copy link
Contributor

Even with the larger IDs, this PR still ends up a net positive to memory usage (54mb on self). I believe it's due to salsa-rs/salsa#888 (but it is unrelated to the larger ID, so we could have an even larger save).

@Veykril
Copy link
Member Author

Veykril commented Jun 13, 2025

Really? From what I saw this regresses memory by ~100 mb

@ChayimFriedman2
Copy link
Contributor

How did you measure? I measured on Linux with jemalloc.

@Veykril
Copy link
Member Author

Veykril commented Jun 13, 2025

I compared the CI runs for analysis-stats from this PR and the parent merge of this PR

@ChayimFriedman2
Copy link
Contributor

I compared the CI runs for analysis-stats from this PR and the parent merge of this PR

???

CI analysis-stats for this PR does not even pass compilation...

@Veykril
Copy link
Member Author

Veykril commented Jun 13, 2025

oh wait I mixed this up with my other PR lol, I did try this locally nevermind 😅 Maybe I made a mistake when switching revisions for comparison if your results are that different

@Veykril Veykril force-pushed the push-rlrsyxsqnxnn branch from 703e21f to 1fb939d Compare June 13, 2025 07:11
@Veykril
Copy link
Member Author

Veykril commented Jun 13, 2025

Wait if you tried this yourself, I assume you also had to fix up some dependencies in salsa right? Cause right now we have a dependency conflict with the new salsa. If you haven't then you didn't test the correct thing.

@ChayimFriedman2
Copy link
Contributor

Yes I did. I just removed Cargo.lock and let Cargo regenerate it.

@Veykril Veykril force-pushed the push-rlrsyxsqnxnn branch from 1fb939d to 38826bc Compare June 13, 2025 07:29
@Veykril
Copy link
Member Author

Veykril commented Jun 13, 2025

okay so the generational index stuff is resolved, we are now marking the 3 interneds that we drop the generation of as eternal so cutting the generation won't cause problems

@Veykril Veykril force-pushed the push-rlrsyxsqnxnn branch from 38826bc to 8cb686b Compare June 13, 2025 07:44
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.

3 participants