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

Merged
merged 1 commit into from
Jul 3, 2025
Merged

Bump salsa #19923

merged 1 commit into from
Jul 3, 2025

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented Jun 4, 2025

No description provided.

@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
@Veykril Veykril mentioned this pull request Jun 18, 2025
@Veykril Veykril force-pushed the push-rlrsyxsqnxnn branch 3 times, most recently from 5dc86db to 055c759 Compare June 27, 2025 09:46
@Veykril Veykril marked this pull request as ready for review June 27, 2025 09:46
@Veykril Veykril force-pushed the push-rlrsyxsqnxnn branch 2 times, most recently from 21b1a1e to 873cb09 Compare June 27, 2025 10:10
@Veykril
Copy link
Member Author

Veykril commented Jun 27, 2025

That test failure is not good, I can't reproduce that on windows ... any mac users here?

@Veykril
Copy link
Member Author

Veykril commented Jun 27, 2025

There is a couple of queries being recomputed here which seems odd. I think this might be ast id collisions happening on mac only ...

@davidbarsky
Copy link
Contributor

That test failure is not good, I can't reproduce that on windows ... any mac users here?

Yeah, I can check in a few hours.

@davidbarsky
Copy link
Contributor

There is a couple of queries being recomputed here which seems odd. I think this might be ast id collisions happening on mac only ...

I think the test failures are also on Linux: https://github.com/rust-lang/rust-analyzer/actions/runs/15923806642/job/44916411005?pr=19923. However, GitHub Actions is reporting that as a cancelled, not failed, action.

@Veykril Veykril force-pushed the push-rlrsyxsqnxnn branch 2 times, most recently from 7223d36 to 261b10b Compare June 28, 2025 07:52
@Veykril Veykril force-pushed the push-rlrsyxsqnxnn branch from 261b10b to d5c40a4 Compare June 28, 2025 07:57
@Veykril
Copy link
Member Author

Veykril commented Jun 28, 2025

Cool, so the test fails on CI on all targets, but not for me locally 🙃

@Veykril Veykril force-pushed the push-rlrsyxsqnxnn branch 3 times, most recently from 6990698 to 3d1e10c Compare June 28, 2025 08:50
@Veykril
Copy link
Member Author

Veykril commented Jun 28, 2025

Okay, the issue is in fact an AstId collision. Now I am unsure why that happens only on CI though, there is nothing environment specific in that code.

@Veykril
Copy link
Member Author

Veykril commented Jun 28, 2025

Welp, I have no idea why the ast id is unstable and I am unable to reproduce this locally at al.


        "DidReuseInternedValue { key: StructId(Id(3000g1)), revision: R6 }",
        "DidDiscard { key: struct_signature_shim(Id(3000g1)) }",
        "DidDiscard { key: struct_signature_with_source_map_shim(Id(3000g1)) }",
        "DidDiscard { key: generic_predicates_shim(Id(3000g1)) }",
        "DidDiscard { key: value_ty_shim(Id(3000g1)) }",
        "DidDiscard { key: VariantFields::firewall_(Id(3000g1)) }",
        "DidDiscard { key: VariantFields::query_(Id(3000g1)) }",
        "DidDiscard { key: adt_variance_shim(Id(3000g1)) }",
        "DidDiscard { key: variances_of_shim(Id(3000g1)) }",
        "DidDiscard { key: type_for_adt_tracked(Id(3000g1)) }",

Specifically this part should not be happening, we are evicting the interned StructLoc here because its hash must've changed which is why I believe the AstId has collided.

@Veykril Veykril force-pushed the push-rlrsyxsqnxnn branch 3 times, most recently from da1f1d7 to 017bae4 Compare June 28, 2025 11:09
@Veykril Veykril force-pushed the push-rlrsyxsqnxnn branch from 017bae4 to 8029c73 Compare July 3, 2025 08:05
@Veykril
Copy link
Member Author

Veykril commented Jul 3, 2025

I disabled the garbage collection for interneds for now as they make our incremental tests fail (due to immediately garbage collecting).

@Veykril Veykril enabled auto-merge July 3, 2025 08:10
@Veykril Veykril added this pull request to the merge queue Jul 3, 2025
Merged via the queue into rust-lang:master with commit 661e7d2 Jul 3, 2025
15 checks passed
@Veykril Veykril deleted the push-rlrsyxsqnxnn branch July 3, 2025 08:28
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants