Skip to content

Conversation

@MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented Oct 31, 2025

This PR changes when salsa calls cycle_fn.

Before: Salsa only called cycle_fn when the last provisional and current value were different (this cycle head hasn't converged)

Now: Salsa will call cycle_fn for every iteration

This feels more consistent for a cycle with nested cycles because, before, a query could have converged but kept being executed
because one of the other heads in this cycle hasn't converged yet.

Always calling the cycle_fn also has the benefit that users can use it to finalize the query result.

This also makes CycleRecoveryAction redundant because there's no real difference between Iterate and Fallback. That's why this PR changes the cycle_fn signature to take an owned value and return Self::Output.

@netlify
Copy link

netlify bot commented Oct 31, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 86aa28f
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/69091835c73265000718b599

@codspeed-hq
Copy link

codspeed-hq bot commented Oct 31, 2025

CodSpeed Performance Report

Merging #1021 will not alter performance

Comparing MichaReiser:always-call-cycle-fn (86aa28f) with master (664750a)

Summary

✅ 13 untouched

@MichaReiser MichaReiser requested review from carljm and ibraheemdev and removed request for ibraheemdev October 31, 2025 21:53
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@MichaReiser
Copy link
Contributor Author

I'd appreciate a thumbs up from @mtshiba on this change before merging, considering that it's a breaking change. I, unfortunately, can't request review from you

@mtshiba
Copy link
Contributor

mtshiba commented Nov 5, 2025

LGTM!

@MichaReiser MichaReiser added this pull request to the merge queue Nov 5, 2025
Merged via the queue into salsa-rs:master with commit 05a9af7 Nov 5, 2025
14 checks passed
@MichaReiser MichaReiser deleted the always-call-cycle-fn branch November 5, 2025 12:47
mtshiba added a commit to mtshiba/ruff that referenced this pull request Nov 5, 2025
mtshiba added a commit to mtshiba/salsa that referenced this pull request Nov 11, 2025
There was actually no need to run `recover_from_cycle` if the query is converged
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