content-sqlite: add group commit to improve performance of bursty stores - #7702
content-sqlite: add group commit to improve performance of bursty stores#7702grondo wants to merge 6 commits into
Conversation
e69ce2b to
5bceeb6
Compare
|
This seems like a nice improvement. Would it be acceptable to defer consideration of this until after we get the next tag out? Assuming we can land online GC, if something goes wrong it may be better to have only one change on the store path to pin it on :-) |
|
Good idea! |
5bceeb6 to
32787e5
Compare
|
On a hunch I tried running I think the pending transaction needs to be flushed on entry to mark and sweep (new since this PR was created). |
|
Ooh. Good catch! I just rebased and fixed conflicts and didn't examine the branch for other needed updates. Let me ask: should we just hold off on this PR for now? It really only helps on the restore case I think. |
|
I guess it's not urgent, but it does seem like a good change. Maybe we can return to it later in the new release cycle? BTW, the error reporting was not awesome in that |
32787e5 to
c9f6e8c
Compare
|
Well, FWIW, I added the missing |
|
group commit is causing an intermittent hang in |
64cf1d9 to
09d97cf
Compare
Ok, I think I found the cause of this, but someone please check carefully. I think the group commit code made it more likely than on current master that a sql commit would return ENOSPC, but a subsequent, smaller commit would not. Therefore the cache could clear a pending The fix (as suggested by Claude) is to only reset I put this commit first in the patch stack to avoid breaking the tests between commits. |
|
New hang in the |
Problem: The content flush error handling assumes each blob commits to the backing store independently, so once stores begin failing they keep failing until recovery. A backing store that batches commits can break this: a batch may fail on `ENOSPC`, then the rollback frees space so a later, smaller store succeeds while earlier failed entries remain dirty. That fools two flush paths into hanging: a store success clears the sticky `flush_errno` so a new content.flush misses the error, and a flush already parked on outstanding stores is never answered when the last store succeeds with dirty entries left over. Clear `flush_errno` only when the cache is fully clean, answer parked flush requests once no stores remain in progress, and report the recorded backing-store errno so callers see the true cause. Assisted-by: Claude:Opus-4.8
Problem: content-sqlite commits every blob in its own implicit transaction. Storing a large number of blobs in a burst (e.g. offline flux-restore of an instance with many inactive jobs) pays per-commit WAL overhead once per blob, which dominates the operation. Add opportunistic group commit. When batch-timeout is nonzero, store inserts are grouped into an explicit transaction. Store requests are not answered until that transaction commits to preserve the guarantee that a store response implies the blob is durable. Batches are committed as soon as no further store request is queued behind the current one by checking flux_pollevents(), so bursts accumulate a large transaction while isolated stores commit immediately without added latency. A batch-timeout age ceiling and an internal count cap bound the size of a batch that would otherwise never drain. The default batch-timeout is set to 0.1s, which had no measurable impact on job throughput workloads, where stores are likely not bursty enough to accumulate. Assisted-by: Claude:Opus-4.8
Problem: There is no way to observe whether group commit is engaging or how effectively it batches stores. Report the current batch_timeout in the stats-get config object, and add a batch_size tstat recorded at each committed transaction. The tstat's count is the number of committed transactions and its mean/max are the stores per transaction (min 1, or count 0 when group commit is disabled). Assisted-by: Claude:Opus-4.8
Problem: There is no test coverage for the new content-sqlite group commit or the reported group commit stats. Add tests to t0012-content-sqlite.t. Assisted-by: Claude:Opus-4.8
Problem: The startup restore of a content dump goes through the content cache, which caps concurrency to the backing store at its flush batch limit and buffers every restored blob in rank 0 memory before draining. Restoring a large dump is therefore much slower than it needs to be. Restore with --no-cache and a deep request window (--maxreqs=100000) so stores go directly to the backing store and arrive as a large burst. Combined with content-sqlite group commit, this collapses hundreds of thousands of per-blob commits into a small number of transactions. These are backing-module-agnostic restore flags, so no module guard is needed. Assisted-by: Claude:Opus-4.8
Problem: The content-sqlite mark and sweep RPCs each open their own transaction and must first flush any open group-commit batch (SQLite has no nested transactions, and the GC pass must see committed table state), but there is no test coverage for this interaction. The one-shot rpc tool cannot reproduce it: each request drains the module recv queue before the next arrives, so a batch is never open when mark/sweep runs. Add t/kvs/content-gc-race, which pipelines a burst of stores followed immediately by a mark or sweep on a single handle, so the GC RPC lands while the store batch is still open. Use timeouts so a reintroduced bug fails promptly rather than hanging CI. Drive it from two new cases in t0043-content-sqlite-gc.t. Assisted-by: Claude:Opus-4.8
09d97cf to
6df4a05
Compare
|
Ok, Claude found the issue in the
Ensure stuck errors are reported correctly back to caller. Note: the single-commit-per-blob assumption in the cache and tools makes this PR a bit risky IMO. The changes here should be carefully reviewed before being approved. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7702 +/- ##
========================================
Coverage 83.91% 83.92%
========================================
Files 587 587
Lines 99979 100082 +103
========================================
+ Hits 83901 83989 +88
- Misses 16078 16093 +15
🚀 New features to boost your workflow:
|
|
I'll open an issue on that cache flush bug. I have a feeling that area of code might benefit from a thorough review. |
2 participants
Problem: content-sqlite commits every blob in its own implicit transaction. Storing a large number of blobs in a burst — for example the offline
flux restoreof a restart dump with many inactive jobs — pays per-commit WAL overhead once per blob, which dominates the operation.This PR adds opportunistic group commit to content-sqlite. When enabled, store INSERTs are grouped into a single explicit transaction, and a store request is not answered until that transaction is fully committed — preserving the guarantee that a store response implies the blob is durable.
A batch is committed as soon as no further store request is queued behind the current one, detected via
flux_pollevents():FLUX_POLLINon the module's handle means another request is already waiting, so the burst has not drained yet and the batch keeps growing. This makes grouping of INSERTs self-tuning — a sustained burst piles into a large transaction, while an isolated store commits immediately with no added latency (no delay is ever introduced to wait for a batch to fill).Two limits are introduced to ensure an ever growing batch has an explicit ceiling:
batch-timeout(default 0.1s) puts an upper limit on the latency and growth of a batchBATCH_COUNT_MAXsets a hard limit of the total number of stores in a batchThe 0.1s default for
batch-timeoutshows no impact on job throughput, but gives a large return for the flux-restore phase (with some caveats, see below)Results:
These results are for a restart dump with 64K inactive jobs → 587,272 objects.
batchstats are stores coalesced percommitted transaction, reported by
flux module stats content-sqlite.--no-cache--no-cache --maxreqs=100000As shown above, simply enabling the group commit default already collapses 587K commits into a few thousand transactions on the normal path. On the cached path the content cache's flush window caps concurrency (max 256/txn). Bypassing the cache and using a large number of outstanding requests lets the backing store see the full burst and coalesce far more aggressively, improving the restore time by ~3x.
Therefore, in this PR, rc1 is adjusted to run
flux restorewith--no-cache --maxreqs=100000to get the full speedup.Note: This PR is probably just a nice-to-have, as we already introduced a 1.5-2x speedup with the sliding store window on
flux restorein #7701. Perturbing the content-sqlite module for gains only influx restoreis probably slightly questionable, and I'm unsure if the grouping will have any measurable effects outside of this one use case (maybe noticed a benefit in the throughput test without real execution). So I won't be surprised if this one is met with some skepticism, especially if Flux won't need to restore on every restart with online GC.