-
Notifications
You must be signed in to change notification settings - Fork 192
pass Cycle to the cycle recovery function
#1028
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
Conversation
This reverts commit cc35b82.
There was actually no need to run `recover_from_cycle` if the query is converged
This is more ergonomic when sharing `Cycle`
✅ Deploy Preview for salsa-rs canceled.
|
CodSpeed Performance ReportMerging #1028 will degrade performances by 5.01%Comparing Summary
Benchmarks breakdown
|
7283d5a to
55810e0
Compare
This reverts commit 23b4ba7.
|
Hmm, it seems like I'm experiencing a consistent regression, but I'm not sure what the direct cause is. |
|
I think you can ignore those, the regressions are in benchmarks that should be irrelevant to your changes so its likely just noise from code location changes |
MichaReiser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, I only have a few nit comments
2bb393d to
ada3ed8
Compare
Currently, the cycle recovery function receives the following arguments:
After some consideration, we realized that we needed to expose the IDs of all cycle heads, which meant an additional argument was added, and the current approach of passing all information about the cycle to the cycle recovery function one by one would no longer be desirable. Cycle information that can be passed independently of the query function signature should be passed as a single context structure.
In this PR, we will expose this context as
Cycleand pass it to the cycle recovery function.