Skip to content

Lazy finalization of cycle participants in maybe_changed_after #854

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 2 commits into from
May 9, 2025

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented May 8, 2025

I'm not entirely sure if we should make this change but it does feel closer to how cycles work in execute. On the other hand, deep_verify_memo now works fairly different for cycles than it does for other queries.

This PR removes the loop from deep_verify_memo. We used the loop to iterate a second time if all dependencies in a cycle remain unchanged. I don't think this is strictly necessary because we can rely on Salsa lazily verifying the inner queries when they're pulled as part of another query (they won't run into a cycle again because it's already verified).

We may want to introduce a new event, e..g DidProvisionallyValidateMemoizedValue (open to suggestions) to make it more apparent that the dependencies were verified, they simply were never "finalized"

I still need to update the comment but I'm interested in what people think first.

Performance

The performance on ty is about the same as the old version

Copy link

netlify bot commented May 8, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 87d4e19
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/681da4e9b6471400084e57f7

Copy link

codspeed-hq bot commented May 8, 2025

CodSpeed Performance Report

Merging #854 will not alter performance

Comparing MichaReiser:cycle-no-deep-verify (87d4e19) with master (af69cc1)

Summary

✅ 12 untouched benchmarks

@MichaReiser MichaReiser marked this pull request as ready for review May 8, 2025 20:04
@MichaReiser MichaReiser requested a review from carljm May 8, 2025 20:42
@carljm
Copy link
Contributor

carljm commented May 8, 2025

Does this make a perf difference in ty?

I think this makes sense.

@MichaReiser
Copy link
Contributor Author

Does this make a perf difference in ty?

There's no change (within the noise margin)

@MichaReiser MichaReiser added the refactoring Code works but is messy label May 9, 2025
@MichaReiser MichaReiser added this pull request to the merge queue May 9, 2025
Merged via the queue into salsa-rs:master with commit a6793e7 May 9, 2025
16 of 17 checks passed
@MichaReiser MichaReiser deleted the cycle-no-deep-verify branch May 9, 2025 07:14
@github-actions github-actions bot mentioned this pull request May 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Code works but is messy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants