Base implementation of scheduled merges #426
Conversation
jorisdral
requested review from
dcoutts,
mheinzel and
recursion-ninja
as code owners
jorisdral
force-pushed
the
jdral/scheduled-merges
branch
from
49eb335 to
c6d3191
Compare
jorisdral
force-pushed
the
jdral/fix-table-content-references
branch
from
92f5b05 to
a06bf8b
Compare
jorisdral
changed the base branch from
jdral/fix-table-content-references
to
jdral/cache-references
jorisdral
force-pushed
the
jdral/cache-references
branch
from
293d1eb to
6472fba
Compare
jorisdral
force-pushed
the
jdral/scheduled-merges
branch
3 times, most recently
from
2cb14b8 to
ed37bdb
Compare
jorisdral
force-pushed
the
jdral/scheduled-merges
branch
2 times, most recently
from
3ac6360 to
a840218
Compare
jorisdral
force-pushed
the
jdral/scheduled-merges
branch
from
a840218 to
6eadec9
Compare
There was a problem hiding this comment.
So since I did not get to clean up this PR yet, I'll leave some comments here for the reviewers. Note that I'm calling this a base implementation because the tests pass, though they are currently failing because of a semi-unrelated problem in control-test (see the last two PRs called WIP: fix failures). There are some optimisations we can still include. Most notably, rebuilding caches on lookups, and batching merging work. We can add those optimisations entirely separately in follow-up PRs
jorisdral
force-pushed
the
jdral/scheduled-merges
branch
from
1e19bbd to
25f0251
Compare
There was a problem hiding this comment.
This is looking very good.
The main thing that makes me a bit nervous is that we track merging steps done, but not credits supplied. My intuition is that like in the prototype, we should track the merge credits we expect we need for a merge, and the number actually supplied, and make sure the credits supplied is equal or greater than the credits required by the time the merge result is expected.
| pure (x, r) | ||
| OngoingMerge _rs _ _m -> do | ||
| -- TODO: should this complete the merge instead, and issue a warning | ||
| -- (e.g., through a trace message)? It is unclear what is best. On the | ||
| -- one hand, it is not a case we can not recover from, because we can | ||
| -- step the merge to completion. On the other hand calling | ||
| -- expectCompletedMerge on an incomplete merge actually means we have | ||
| -- a bug in our scheduling, since we take great care to ensure merges | ||
| -- always finish on time. | ||
| error "expectCompletedMerge: expected a completed merge, but found an ongoing merge" |
There was a problem hiding this comment.
In debug-mode, this should fail, so using assert would make sense. In release mode, I think it would be sensible to complete the merge and log something. Logging a warning is a bit different than what we otherwise do with the tracer, but it seems like our best option for that.
There was a problem hiding this comment.
So there seems to be some disagreement
jorisdral
force-pushed
the
jdral/scheduled-merges
branch
from
5501499 to
58a8c73
Compare
jorisdral
force-pushed
the
jdral/scheduled-merges
branch
from
58a8c73 to
6b3a366
Compare
In some cases, we have to remove more than one reference at a time. For example, when a merge is completed, then the reference count of each input run should be reduced by value of the reference count of the output run. You can view this as the references being "transferred" from the input runs to the output run.
jorisdral
force-pushed
the
jdral/scheduled-merges
branch
3 times, most recently
from
abc1e07 to
bb95545
Compare
The following features will be implemented later: rebuilding caches on lookups, and doing merging work in batches.
And test with `Incremental` in the `StateMachine` tests.
jorisdral
force-pushed
the
jdral/scheduled-merges
branch
from
bb95545 to
27107f3
Compare
jorisdral
dismissed
mheinzel’s stale review
Resolved comments, and other things that might come up will be resolved in #435
|
🎉 |
No description provided.