Skip to content

Commit 095cdbb

Browse files
committed
The credit scaling in the impl was backwards from the prototype
The credit scaling for levelling vs teiring use different formulas. The implementation's version of this scaling had the two cases swapped. This will have resulted in supplying too few credits to levelling runs (and thus having to complete too much at the end) and too many to teiring runs (thus completing too early). In both cases it will have resulted in IO not being spread out evenly, and thus higher peak latency for updates. We can now successfully assert that upon expecting a merging run to be complete we have supplied credits equalling the total debt. Previously this assertion failed, which led to the discovery of the bug. We need to find a test to ensure we are spreading out merging work and not supplying too many credits. We supply credits for the worst case, which currently means we supply excess in typical cases. We should scale the credit supply to the actual debt for each merge, based on the size of the input runs.
1 parent 74d9048 commit 095cdbb

File tree

2 files changed

+16
-17
lines changed

2 files changed

+16
-17
lines changed

src/Database/LSMTree/Internal/MergeSchedule.hs

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -884,17 +884,14 @@ supplyCredits conf c levels =
884884
-- on the merging run.
885885
--
886886
-- Initially, 1 update supplies 1 credit. However, since merging runs have
887-
-- different numbers of input runs/entries, we may have to a more or less
887+
-- different numbers of input runs\/entries, we may have to a more or less
888888
-- merging work than 1 merge step for each credit.
889-
scaleCreditsForMerge :: MergePolicyForLevel -> Ref (MergingRun m h) -> Credits -> MR.Credits
890-
-- A single run is a trivially completed merge, so it requires no credits.
891-
scaleCreditsForMerge LevelTiering _ (Credits c) =
892-
-- A tiering merge has 5 runs at most (one could be held back to merged
893-
-- again) and must be completed before the level is full (once 4 more
894-
-- runs come in).
895-
MR.Credits (c * (1 + 4))
896-
897-
scaleCreditsForMerge LevelLevelling mr (Credits c) =
889+
scaleCreditsForMerge ::
890+
MergePolicyForLevel
891+
-> Ref (MergingRun m h)
892+
-> Credits
893+
-> MR.Credits
894+
scaleCreditsForMerge LevelLevelling _ (Credits c) =
898895
-- A levelling merge has 1 input run and one resident run, which is (up
899896
-- to) 4x bigger than the others. It needs to be completed before
900897
-- another run comes in.
@@ -904,6 +901,11 @@ scaleCreditsForMerge LevelLevelling mr (Credits c) =
904901
-- worst-case upper bound by looking at the sizes of the input runs.
905902
-- As as result, merge work would/could be more evenly distributed over
906903
-- time when the resident run is smaller than the worst case.
904+
MR.Credits (c * (1 + 4))
905+
scaleCreditsForMerge LevelTiering mr (Credits c) =
906+
-- A tiering merge has 5 runs at most (one could be held back to merged
907+
-- again) and must be completed before the level is full (once 4 more
908+
-- runs come in).
907909
let NumRuns n = MR.numRuns mr
908910
-- same as division rounding up: ceiling (c * n / 4)
909911
in MR.Credits ((c * n + 3) `div` 4)

src/Database/LSMTree/Internal/MergingRun.hs

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -765,13 +765,10 @@ expectCompleted (DeRef MergingRun {..}) = do
765765
when (knownCompleted == MergeMaybeCompleted) $ do
766766
(SpentCredits spentCredits,
767767
UnspentCredits unspentCredits) <- atomicReadCredits mergeCreditsVar
768-
let !totalDebt = numEntriesToTotalDebt mergeNumEntries
769-
!suppliedCredits = spentCredits + unspentCredits
770-
!credits = assert (suppliedCredits <= totalDebt) $
771-
totalDebt - spentCredits
772-
--TODO: the following ought to be true and be the right answer:
773-
-- !credits = assert (suppliedCredits == totalDebt) $
774-
-- unspentCredits
768+
let totalDebt = numEntriesToTotalDebt mergeNumEntries
769+
suppliedCredits = spentCredits + unspentCredits
770+
!credits = assert (suppliedCredits == totalDebt) $
771+
unspentCredits
775772

776773
--TODO: what about exception safety: check if it is ok to be interrupted
777774
-- between performMergeSteps and completeMerge here, and above.

0 commit comments

Comments
 (0)