Resurrect UTxO statistics in LSM-trees #1810
Conversation
jasagredo
requested review from
bladyjoker,
dnadales,
fraser-iohk,
geo2a and
nfrisby
as code owners
| Diff.Insert{} -> True | ||
| Diff.Delete -> False | ||
| ) | ||
| diffs |
There was a problem hiding this comment.
You're iterating over diffs in the loop above, consider making these calculations there.
There was a problem hiding this comment.
Easier said than done. The above is inside a Vec.create loop 🤔
There was a problem hiding this comment.
You decide, if you're expecting thousands of actions might be worth the effort :) This is quite readable as is.
There was a problem hiding this comment.
The issue is that create cannot return another value next to the vector, so we have to create the insertions and deletions outside of the create loop.
jasagredo
force-pushed
the
js/resurrect-utxo-count
branch
2 times, most recently
from
a69d66a to
972437d
Compare
jasagredo
force-pushed
the
js/leaks-v2
branch
from
7b0f9ab to
c8c453b
Compare
jasagredo
force-pushed
the
js/resurrect-utxo-count
branch
from
972437d to
488eccf
Compare
jasagredo
force-pushed
the
js/leaks-v2
branch
from
c8c453b to
54a30dd
Compare
jasagredo
force-pushed
the
js/resurrect-utxo-count
branch
from
488eccf to
c017ac5
Compare
dnadales
left a comment
dnadales
left a comment
There was a problem hiding this comment.
Great to see support for tracking utxo size in LSM 🙌
Some additional questions:
- Do we need a changelog entry?
- Would it make sense to add invariants to check that we're computing the size correctly?
- Should we add tests to check the counting logic works as expected?
| , IndexedMemPack (l EmptyMK) (TxOut l) | ||
| ) => | ||
| Tracer m LedgerDBV2Trace -> | ||
| Int -> |
There was a problem hiding this comment.
Do we want to add some comment on this parameter?
There was a problem hiding this comment.
Don't we want to use an unsigned type for the table size?
| Tracer m LedgerDBV2Trace -> | ||
| m (ResourceKey m, LedgerTablesHandle m l) | ||
| implDuplicate rr t tracer = do | ||
| implDuplicate rr sz t tracer = do |
There was a problem hiding this comment.
Maybe we can expand this to size or tSize
| Tracer m LedgerDBV2Trace -> UTxOTable m -> l mk -> l DiffMK -> m () | ||
| implPushDiffs tracer t _ !st1 = | ||
| Tracer m LedgerDBV2Trace -> UTxOTable m -> StrictTVar m Int -> l mk -> l DiffMK -> m () | ||
| implPushDiffs tracer t s _ !st1 = |
There was a problem hiding this comment.
Would it make sense to use a unique name for the size variable throughout the code base?
| S.effects s' | ||
| Just (item, s'') -> go tb (n - 1) (item : m) s'' | ||
| (,tbsSize) <$> S.effects s' | ||
| Just (item, s'') -> go (tbsSize + 1) tb (n - 1) (item : m) s'' |
There was a problem hiding this comment.
I'm not entirely sure what are we computing here, and why are we incrementing and decrementing the variables here. Maybe some more representative variable names and comments could help.
|
|
||
| readUTxOSizeFile :: MonadThrow m => HasFS m h -> FsPath -> ExceptT (SnapshotFailure blk) m Int | ||
| readUTxOSizeFile hfs p = | ||
| fmap fst $ |
There was a problem hiding this comment.
Don't we want to check there's no trailing data?
Also we might want to add some validation that:
- the size is non-negative
- the file exists.
| Monad.void $ withFile hasFs p (WriteMode MustBeNew) $ \h -> | ||
| hPutAll hasFs h $ BS.toLazyByteString $ BS.intDec sz | ||
|
|
||
| readUTxOSizeFile :: MonadThrow m => HasFS m h -> FsPath -> ExceptT (SnapshotFailure blk) m Int |
There was a problem hiding this comment.
How do we want to handle the case in which the utxoSize does not exist for old snapshots? Do we invalidate them?
| Diff.Delete -> False | ||
| ) | ||
| diffs | ||
| atomically $ modifyTVar s (\x -> x + ins - dels) |
There was a problem hiding this comment.
Couldn't we simply fold?
let (ins, dels) = Map.foldl'
(\(i, d) delta -> case delta of
Diff.Insert{} -> (i+1, d)
Diff.Delete -> (i, d+1)
)
(0, 0)
diffs
atomically $ modifyTVar s (\x -> x + ins - dels)
LSM-trees has no way to easily tell the number of entries in a table. With this PR we keep a counter that we modify as we push differences to the tables.
This was requested by the Performance and Tracing team.