-
Notifications
You must be signed in to change notification settings - Fork 7
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
Prototype: change credit scaling, using nominal vs physical credit #571
Open
dcoutts
wants to merge
7
commits into
main
Choose a base branch
from
dcoutts/merge-credits-scaling
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+410
−213
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Consolidate it and move it before first use (in merging run).
Change it from tracking just the unspent credits and remaining debt to tracking the spent and unspent credits and the total debt. This makes it more similar to the tracking we do in the real implementation. It also makes it easier to assert things: like the supplied credit being less than the total debt. And finally, we will need to know the total debt for the nominal/physical credit scaling that we want to try out.
So we split out the merge debt (which never changes) and keep it in the MergingRun, so that it is accessible directly and purely for both ongoing and completed merges. Previously the merge debt was only available for the OngoingMerge, not the CompletedMerge, and one had to read the STRef MergingRunState to get it. We'll want to know the total merge debt directly to help simplify how we supply credits.
Use a dedicated type that does not contain extranious information. There will soon be even more extranious information that makes sense only for merges within levels, and not within merging trees, so now is a good time to separate it before it becomes too complex. In particular there would be no sensible way to generate the unneccessary information for property tests.
Previously we've had a somewhat complex method to scale credits from those supplied in a level (1 per update) to the credits used in merging. The existing scheme has a number of downsides: * no clear distinction between scaled and unscaled credits, what does each one measure? * complex, needs scaling dependent of the merge-policy * always uses worst case supply of credits so often finishes early * rounding errors compound problem of credit over-supply * no satisfactory way to check we do not over-supply credits * contributing credits to a merge from multiple handles will cause the merge to finish earlier than any of the handles needs it to finish, thus doing work more eagerly than necessary. The new scheme involves: * distinguishing physical vs nominal credits, with a clear definition for each measure * converting between nominal and physical without merge policy * physical debt is no longer worst case but matches actual cost * integer rounding errors do not compound * we can assert that we reach the nominal and physical debt totals at the same moment (when the merge is done) and thus we can assert no over-supply of physical credits. * we can avoid supplying credits too quickly to a merge that is shared between multiple handles, each one only pushes it as far as it needs.
mheinzel
approved these changes
Feb 13, 2025
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.
Nice.
Move assertions out of line and factor out common definitions.
702031b
to
0487ec5
Compare
@mheinzel PR review applied in two extra patches. If you think that's good now then I'll squash and merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously we've had a somewhat complex method to scale credits from
those supplied in a level (1 per update) to the credits used in merging.
The existing scheme has a number of downsides:
each one measure?
merge to finish earlier than any of the handles needs it to finish,
thus doing work more eagerly than necessary.
The new scheme involves:
for each measure
the same moment (when the merge is done) and thus we can assert no
over-supply of physical credits.
between multiple handles, each one only pushes it as far as it needs.