Do merge work in batches #435
Conversation
jorisdral
requested review from
dcoutts,
mheinzel and
recursion-ninja
as code owners
jorisdral
force-pushed
the
jdral/batch-merge-work
branch
from
9e80b61 to
0fdf091
Compare
jorisdral
force-pushed
the
jdral/scheduled-merges
branch
6 times, most recently
from
bb95545 to
27107f3
Compare
jorisdral
force-pushed
the
jdral/batch-merge-work
branch
2 times, most recently
from
0d7efcb to
5309353
Compare
mheinzel
left a comment
mheinzel
left a comment
There was a problem hiding this comment.
Looks good, nice documentation! Just a few points to discuss.
jorisdral
force-pushed
the
jdral/batch-merge-work
branch
4 times, most recently
from
0d7e461 to
4837e51
Compare
mheinzel
left a comment
mheinzel
left a comment
There was a problem hiding this comment.
Looks good, but some additional questions.
mheinzel
left a comment
mheinzel
left a comment
There was a problem hiding this comment.
I think the naming can be clarified a bit, but should work!
jorisdral
force-pushed
the
jdral/batch-merge-work
branch
from
c5cc13a to
78d6413
Compare
mheinzel
left a comment
mheinzel
left a comment
There was a problem hiding this comment.
I think this is alright now. Just some small tweaks.
mheinzel
left a comment
mheinzel
left a comment
There was a problem hiding this comment.
I also think the scaled/unscaled credit distinction could be a little clearer in some places, but this will improve as soon as we move MergingRun into its own module. In there, we will always consider scaled credits. In MergeSchedule we talk about unscaled credits, only converting at the end to call MergingRun.supplyMergeCredits.
|
|
||
| data SnapMergingRun = | ||
| SnapMergingRun !MergePolicyForLevel !NumRuns !SnapMergingRunState | ||
| SnapMergingRun !MergePolicyForLevel !NumRuns !NumEntries !UnspentCredits !MergeKnownCompleted !SnapMergingRunState |
There was a problem hiding this comment.
Do we need to snapshot the MergeKnownCompleted? It's like a cache, we can just reconstruct it from the SnapMergingRunState.
There was a problem hiding this comment.
I've thought about this as well, and I think both with and without storing it here works. It just means we can skip a bit of work now
There was a problem hiding this comment.
I think I'd rather keep the snapshot format lean. This would just mean two more lines of code in opening a snapshot and even save one when creating it I think, or are you referring to some other work?
There was a problem hiding this comment.
I understand your point, but I'm going to change this code later anyway, so I'll keep it as is and follow up with refactorings
jorisdral
force-pushed
the
jdral/batch-merge-work
branch
from
78d6413 to
ea34fe1
Compare
mheinzel
left a comment
mheinzel
left a comment
There was a problem hiding this comment.
I think some previous comments are still unresolved, but no blockers.
| mergeKnownCompleted <- readMutVar mergeKnownCompletedVar | ||
| -- The merge is not guaranteed to be complete. | ||
| when (mergeKnownCompleted == MergeMaybeCompleted) $ do |
There was a problem hiding this comment.
I don't think using the cache matters much here (this only happens once per merge). To keep complexity low, I would only use it on the hot path.
There was a problem hiding this comment.
I'd argue that it decreases complexity: the cached value has a simple correctness argument, so checking it here means we can skip the more complex bit of trying to finish a (maybe) incomplete merge
There was a problem hiding this comment.
Not sure, we need to have the branch that finishes an incomplete merge either way. So this just adds an additional branch. But I don't mind much, all good.
This is done to make it clear that there is a difference between progressing the merge until there are no more inputs to merge (`Done`), and converting the merge into an output run (`Complete`).
See `Note [Credits]`
jorisdral
force-pushed
the
jdral/batch-merge-work
branch
from
0f1b0f9 to
9c4dd1f
Compare
This is a follow-up to #426
The best description of the approach can be found in the code, in the GHC-style note
Note [Credits]