High-throughput, double buffered mempool - #2148
Conversation
|
This is quite well contained .. should this maybe become the first Leios specific change we can production grade by targeting |
d56605d to
0de519d
Compare
|
|
||
| -- * Initial parameters | ||
| , initialLedgerState | ||
| , mkInitialLedgerState |
There was a problem hiding this comment.
(This Comment is here only because this line is rendered nearest the commit message.)
The Add mempool-state-bench: concurrent shared-state access benchmark commit message includes
Adds mkInitialLedgerState + advanceTip helpers to the bench TestBlock,
and docs/mempool-double-buffer-plan.md describing the design + plan.
But, eg, I don't see a docs/mempool-double-buffer-plan.md in the PR.
Would you ask Claude to remove such "dangling" references to your conversation from this PR's commit messages and code comments, since this PR doesn't retain the context of your conversation with Claude?
| -- | Move the tip to a fresh point (distinct per @n@) while keeping the ledger | ||
| -- tables unchanged. Used to force the mempool to resync/revalidate against a | ||
| -- "new" tip without invalidating any of its transactions. | ||
| advanceTip :: |
There was a problem hiding this comment.
Is this function ever called on its own result? Without paging the TestBlock context back into my brain, this function's RHS seems very bogus. I could imagine that's OK, if the function's output is never retained in some accumulating state.
If it doesn't accumulate, please rename it in a way that no longer suggests its accumulative. And maybe point out in this comment where/when it's result is overwritten.
| -- a node under tx-submission load: | ||
| -- | ||
| -- * __Adders__ (tx-submission clients): each submits an independent chain of | ||
| -- transactions via the real 'addTx', rate-limited to a target TPS (like the |
There was a problem hiding this comment.
"tx-firehose" is a foreign concept to this codebase on the main branch. Your top-level comment suggested that maybe this code is already mainnet ready, but if so, then this comment would need to be more specific (eg make reference to the "Leios testnet's tx-firehose").
| -- * __Readers__ (tx-submission servers / forging): tight loop of the real | ||
| -- 'getSnapshot' (@readTMVar istate@), measuring how long a read blocks. | ||
| -- | ||
| -- * __Syncer__ (the mempool sync thread): periodically advances the ledger tip |
There was a problem hiding this comment.
Do we want periodically, or do we want some random variance in the time between occurrences?
| -- and runs the real 'testSyncWithLedger', which revalidates the whole mempool | ||
| -- through the latency-injected forker while holding the state lock. | ||
| -- | ||
| -- The goal of this first version is to reproduce the baseline: the mempool |
There was a problem hiding this comment.
Note to self: check if this comment remains at the tip of the PR. (I'm going commit by commit.)
| isVar <- | ||
| newTMVarIO $ | ||
| initInternalState capacityOverride TxSeq.zeroTicketNo cfg slot st' | ||
| is0 = initInternalState capacityOverride TxSeq.zeroTicketNo cfg slot st' |
There was a problem hiding this comment.
I'd rather not bind a variable name that isn't used anywhere else in the multi-line scope
| let txt = T.pack $ "MempoolTxTooSlow (" <> show dur <> ") " <> show (txId tx) | ||
| in mkMempoolApplyTxError (isLedgerState is) txt | ||
| case mbX of | ||
| res <- case mbX of |
There was a problem hiding this comment.
I'd rather not bind a variable name that isn't used anywhere else in the multi-line scope
| -- lock, so readers block for the merge but not for the big LedgerDB read. | ||
| go = do | ||
| MempoolLedgerDBView ls0 meFrk0 <- atomically $ getCurrentLedgerState ldgrInterface | ||
| is0 <- atomically $ readTMVar istate |
There was a problem hiding this comment.
A little comment please explaining why this atomically is safe/useful to separate from the preceding atomically
| traceWith trcr TraceMempoolTipMovedBetweenSTMBlocks | ||
| go | ||
| Right frk -> do | ||
| -- OFF-LOCK: big read of the snapshot's input values at the new tip. |
There was a problem hiding this comment.
For LSM, I would imagine this could indeed be big.
But the Leios testnet isn't running LSM, is it?
... maybe with EB-sized Mempools, this read is "big" even without LSM?
| castLedgerTables <$> roforkerReadTables frk (castLedgerTables deltaKeys) | ||
| let allValues = ltliftA2 unionValues values0 valuesDelta | ||
| (isFinal, mTrace) = | ||
| pureSyncWithLedger capacityOverride cfg slot ls' allValues isNow |
There was a problem hiding this comment.
This does all of the validation calculation while holding the lock. Maybe subsequent commits move this outside the lock too?
Edit: yes, the next commit does.
Therefore: please refine the title of this commit to clarify that it's only pulling the UTxO HD read outside of the lock so far.
| -- assembled by the /same/ 'buildRevalidatedIS' over the concatenated survivors. | ||
| -- This lets a mempool sync shrink its outstanding work off the lock and take | ||
| -- the lock only for a small, bounded final delta ('implSyncWithLedger'). | ||
| extendReapply :: |
There was a problem hiding this comment.
It occurs to me that I was overly confident during our discussion in the Office Hours yesterday.
In particular, I had said that syncWithLedger was the only thing that can delete txs from the Mempool. But that's wrong: there's another way that txs can be removed.
Point being: the uses of extendReapply is correctly incorporating txs that were added to the Mempool since the last time it looked, BUT it's also incorrectly assuming that any tx it has already successfully revalidated is still in the mempool.
The other API.hs entrypoint that can delete is removeTxsEvenIfValid and its called from the forging loop in the NodeKernel.hs.
Maybe it's fine to "resurrect" those deleted txs on the Leios prototype, but it it'd require much more consideration for the main branch.
There was a problem hiding this comment.
I think applyMempoolDiff is causing the "resurrected" txs to prevent the syncer from adding conflicting txs to its wip Mempool, ie if while the syncer was running tx X was deleted from the Mempool and a tx Y that conflicts with X was added to the Mempool.
The result would be that the syncer would still have X in its wip Mempool and so Y would fail to revalidate.
So at least resurrection doesn't risk creating a Mempool that contains conflicting txs 👍
Edit: instead of resurrection I should say lost/undone deletions.
There was a problem hiding this comment.
In my (second) Review summary, I said
In regards to write-race with
removeTxsEvenIfValid, maybe we haveremoveTxsEvenIfValidraise a signal that causes the syncer to restart from scratch? TheremoveTxsEvenIfValidcalls should be very infrequent.
Another option---one that doesn't leverage the infrequency---would be to have a "outstanding changes" variable that contains a sequence of Either TicketNo [TicketNo]. Each Let would be one addTx success. Each Right would be one removeTxsEvenIfValid success.
Whenever the syncer thread commits its new Mempool finishes one "round" of calculations, it would set the outstanding changes to the empty sequence, since it would have already integrated them all to its work-in-progress Mempool. Whenever the sync thread wasn't running, adds/removes would not extend the sequence, since there is no work-in-progress Mempool---so the sequence variable only needs to exist while the sync thread is running.
I doubt it's worth the trouble, but hopefully spelling out what a "non-degenerate" mechanism would look like helps ensure we're on the same page.
nfrisby
left a comment
There was a problem hiding this comment.
OK, now I've also reviewed the core commit.
The logic looks good to me, with two caveats.
- clobbering the deletions done by
removeTxsEvenIfValid(hence the Request Changes) - There's quite a few moving pieces... so maybe I missed something.
In regards to write-race with removeTxsEvenIfValid, maybe we have removeTxsEvenIfValid raise a signal that causes the syncer to restart from scratch? The removeTxsEvenIfValid calls should be very infrequent.
| modifyMVar_ forkerMVar (\frkOld -> roforkerClose frkOld >> pure frk) | ||
| whenJust mTrace (traceWith trcr) | ||
| pure (Just (projectResult isFinal), isFinal) | ||
| let RevalidateTxsResult cand0 removed0 = |
There was a problem hiding this comment.
I'd suggest a little comment here that signposts this let's calculation as what should be the tallest tent pole, the subsequent shrinkThenCommit is just the catch-up loop to recover from/catch-up to whatever events were missed during that calculation.
There was a problem hiding this comment.
Also: do these variables need bang patterns? Or an evaluate if you prefer? Something to ensure the CPU work is done before we start the shrinkThenCommit loop (which begins by rereading istate)?
| deltaValues <- readDeltaValues deltaTickets | ||
| let RevalidateTxsResult cand' removed' = | ||
| extendReapply capacityOverride cfg slot ls' cand deltaValues (isLastTicketNo isNow) deltaTickets | ||
| shrinkThenCommit cand' (removedAcc ++ removed') (iterN + 1) |
There was a problem hiding this comment.
Maybe we want to emit a tracer here, for each step of "resync" progress?
| else withTMVarAnd istate (const $ getCurrentLedgerState ldgrInterface) $ | ||
| \isLocked (MempoolLedgerDBView ls _meFrk) -> | ||
| if getTipHash ls /= getTipHash ls0 | ||
| then -- Tip moved while we worked; retry the whole sync. |
There was a problem hiding this comment.
We probably want to trace this event, now that it's much more likely.
Models the proto-devnet baseline mempool contention against the real mempool (openMempoolWithoutSyncThread) over a latency-injected mocked ledger interface. Concurrent roles mirror a node under tx-submission load: one server (reader) + one client (adder) per peer, local firehose clients, and a syncer that revalidates the whole mempool while holding the istate lock. Validated against the frozen baseline (V1, 100ms/50Mbit, V2LSM): - low load (100 TPS): ~90 tx/s, keeps up (devnet ~93) - backpressure ceiling ~550 tx/s (offered 2000 == unbounded) - high load (~40k txs): ~8s sync stall, matching the devnet's ~8s intake gaps; sync cost linear at ~200us/key - server-read stall == sync time at every load point (the contention the double-buffer design targets) Adds mkInitialLedgerState + advanceTip helpers to the bench TestBlock, and docs/mempool-double-buffer-plan.md describing the design + plan.
The TestBlock's reapplyTx routed through applyTx, so full validation and reapplication cost the same and were both essentially free. That let the mempool-state-bench degenerate: with apply ~= reapply the sync-vs-ingest convergence assumption is vacuous. Give applyTx a configurable simulated CPU cost (MEMPOOL_APPLY_CPU_US, default 0 so ordinary tests and the criterion mempool-bench are unaffected) via a busy-wait, and run reapplyTx through the ledger transition directly with a much smaller MEMPOOL_REAPPLY_CPU_US so it no longer pays the validation cost. Setting e.g. 200us/20us reproduces the real-node relationship where a mempool sync (reapply-only) is markedly cheaper per tx than ingestion (full validation).
Drop the explicit withMaxSuccess (was 10000) on the atomic parallel test so it uses the default test count (100), overridable from the CLI via --quickcheck-tests, and reduce the reruns from 100 to 10. At the original size the QSM parallel-history linearizability search OOMs (which is why it was disabled); at this size it does not. Rationale for using the default count rather than a fixed one: this test's runtime is highly variable (seconds to minutes), because some generated parallel programs contain long transaction chains whose linearization search is expensive. Leaving the count at the CLI-overridable default lets it be tuned per environment without editing the source. Still disabled here (see the next commit); this commit only resizes it.
The mempool kept its InternalState in a single StrictTMVar used as both data cell and lock: adds, the full re-sync on tip change, removes, and every snapshot reader all contended on it. On a tip change the sync revalidated the entire mempool (reading all inputs from the LedgerDB) while holding the lock, stalling both intake and serving for the whole revalidation — cost growing with occupancy. Instead, the sync thread does its large LedgerDB read off the lock (against a non-emptying 'readTMVar' snapshot serving as the "off screen buffer") and only takes the lock for the reading and reappling the delta txs, so a reader blocks for that sub-second merge but not for the big read. If that merge grows too costly under higher load, the merge itself can later be moved off the lock behind an optimistic retry; the single-cell structure here is the clean baseline.
The single-TMVar sync revalidated all current txs under the lock, so the lock hold — and hence how long snapshot readers (forging, tx serving) can block — grew with mempool occupancy (seconds at tens of thousands of txs). Do the revalidation off the lock and take the lock only for a small, bounded residual. 'implSyncWithLedger' now revalidates the snapshot's txs off the lock, then loops reading the txs added since (the delta, by TicketNo) and reapplying just those on top via the new 'extendReapply'. The delta shrinks each round (adds are serialised and pay full validation, reapplication is cheaper), so once it is at most 'syncDeltaCap' — or after 'syncMaxIters' as a safety valve — we take the lock and reapply only that bounded residual before swapping. The lock hold is thus ~constant (O(syncDeltaCap)) rather than O(occupancy). 'extendReapply' reapplies a delta on top of an already-revalidated state, seeded from its ledger state via 'applyMempoolDiffs', and assembles the result with the same 'buildRevalidatedIS' that 'revalidateTxsFor' uses, so it is byte-identical to a single revalidation of the concatenation (verified by the atomic QSM linearizability test). The candidate is committed via the TMVar, so unlike an optimistic swap it cannot starve. Measured (mempool-state-bench, apply 200us / reapply 20us): max reader stall at ~56k txs drops from ~2.1s to ~300ms, and no longer scales linearly with occupancy; throughput and sync count unchanged.
Real tx servers would not run in a busy loop, but have some work to do between getSnapshot's
253e0d2 to
6264b6f
Compare
The mempool keeps its
InternalStatein a singleStrictTMVarthat is both the data cell and the writer lock: adds, removes, the tip-change re-sync, and every snapshot reader (block forging, tx-submission serving) all go through it. On a tip change the sync revalidates the entire mempool — reading every input from the LedgerDB — while holding the lock, stalling both intake and serving for the whole revalidation. That cost grows with occupancy, and becomes a bottleneck under Leios, where the mempool holds ~2 endorser blocks' worth of transactions and is fed at up to ~1000 tx/s.This PR double-buffers the sync: the expensive work runs off the lock, and the lock is taken only for a small, bounded merge. It's consisting of two parts right now:
Read off the lock: The sync snapshots the state with a non-emptying
readTMVar— the "off-screen buffer" — and does the big LedgerDB read and revalidation of that snapshot off the lock, while adds keep flowing. It then takes the lock only to merge in the transactions added meanwhile (the delta, byTicketNo). Readers block for that merge, not for the big read.Bound the merge: The merge itself is moved off the lock behind a converging loop: reapply just the delta on top of the revalidated snapshot via the new
extendReapply, then re-read the new delta and repeat. The delta shrinks each round — adds are serialised (the fifoMVars) and pay full validation, whereas the sync only reapplies, which is cheaper per tx. Once the delta is ≤syncDeltaCap(or aftersyncMaxIters, a safety valve), the lock is taken to reapply the bounded residual and swap. The lock is thus held forO(syncDeltaCap), notO(occupancy).Correctness
extendReapplyreapplies a delta on top of an already-revalidated state (seeded from its ledger state viaapplyMempoolDiffs) and assembles the result with the samebuildRevalidatedISthatrevalidateTxsForuses — so it is byte-identical to a singlerevalidateTxsForover the concatenation.TMVar(a blocking acquire, not an optimistic compare-and-swap), so the sync always makes progress and cannot be starved by a stream of concurrent adds.extendReapplypath whenever an add interleaves before the sync's lock acquire. Full mempool group (sequential / atomic / non-atomic / fairness) passes.Benchmark
This PR also adds a
mempool-state-benchthat drives the real mempool under concurrent adders, readers and a syncer. Forker-read latency is modelled at500us + 200us/key;applyTxcarries a simulated full-validation CPU cost thatreapplyTxskips (MEMPOOL_APPLY_CPU_US/MEMPOOL_REAPPLY_CPU_US), set to200us / 20usto match a ~217us/tx apply measured on the proto-devnet. Each reader models one downstream peer's tx-submission server, reading on a ~150ms cadence — calibrated from the proto-devnet, where downstream peers pulled ~3–4 tx-body requests/s from a node (so ~5–8 mempool reads/s per peer).Behaviour across 3 orders of magnitude of offered load × 3 peer counts (20s runs). Max snapshot-reader stall — how long a rebase can delay serving or forging — stays bounded everywhere:
Throughput over the same matrix (tx/s) — tracks offered load up to the mempool's serialised-validation ceiling (~630 tx/s at
200us/tx), independent of peer count:The reader stall stays ≤~80 ms across a 100× load range and a 50× peer range; peer count does not inflate it, because all readers wait on the same bounded merge. Without the converging loop, the same stall is the full under-lock revalidation and grows with occupancy instead — e.g. at ~56k txs it is ~2.1 s versus ~0.3 s with the loop.
syncDeltaCap/syncMaxItersare constants inUpdate.hs.