-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
base: master
Are you sure you want to change the base?
Bump salsa #19923
Conversation
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 |
Yeah there is also a problem with placeholders which are |
Actual, syntax context isnt really an issue, as we can (and already are) fall back to the TokenId server model. |
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). |
Really? From what I saw this regresses memory by ~100 mb |
How did you measure? I measured on Linux with jemalloc. |
I compared the CI runs for analysis-stats from this PR and the parent merge of this PR |
??? CI |
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 |
703e21f
to
1fb939d
Compare
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. |
Yes I did. I just removed Cargo.lock and let Cargo regenerate it. |
1fb939d
to
38826bc
Compare
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 |
38826bc
to
8cb686b
Compare
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)