Repository navigation
Conversation
N merges into N independent targets at once against a matched single merge on the same graph, with a control before and after the batch. Each phase opens a fresh handle outside its clock; the batch is released by a barrier and its clock starts before the release. A fresh handle then checks exact row counts on every branch and reads merged keys back. concurrent-writes and concurrent-merges now share the run root, the watchdog and the attestation block.
Every merge's receipt must name its target's head commit, with its source's head as the merged parent; the record counts the receipts checked.
…rges concurrent-writes' exit-78 refusal and exit-75 watchdog moved into the shared run-target module, which the contract now reads, and it requires concurrent-writes to use that module. concurrent-merges gets its own contract: one-shot labeling, the barrier release with the batch clock started before it, fresh-handle verification with receipts, and harness registration.
ragnorc
left a comment
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: approve. I found no blocking defect in 1f11b15dd201fa234128ff706bc0bc290586124f. The inline suggestions are optional. This is a COMMENT review because the authenticated account is the PR author.
What this PR does, in plain words
This PR adds a repeatable way to ask whether separate branch merges run together or wait behind each other. It creates matching source and target branches, then merges several pairs through one shared engine handle. Two separate merges, before and after the batch, provide a comparison. The program checks the resulting graph before it reports success. It also shares fixture setup and reporting helpers with the existing concurrent-writes benchmark. It changes no production engine behavior.
This instrument can help evaluate lock changes such as #825. It cannot, by itself, establish server throughput, production latency, or a safe concurrency limit.
Contract and code checks
The relevant contract is successful, atomic graph publication for each merge. The benchmark must not report a speed improvement when merges fail or return incomplete results. Its workload uses one writer process, one table, short branch histories, and conflict-free inserts. Every merge has its own source and target. This avoids the exclusive branch gates that deliberately serialize merges sharing either branch.
I checked the following paths:
- Session cloning retains the same
Arc<Omnigraph>. The batch therefore exercises shared-handle contention. - Both branches receive distinct keys after their fork. The fixture needs a three-way merge, and the outcome check rejects fast-forward or no-op results.
- The batch clock starts before barrier release. Each task records its completion before the parent joins it. Join order therefore does not change completion times.
- Fresh-handle verification checks every branch's row count, two source keys per target, and each receipt. The engine puts the captured source head into the merge's lineage intent. The public commit listing returns newest first, as the verifier assumes.
- Merge errors or verification failures make the child and parent exit unsuccessfully. Each repetition uses a separate child process.
I checked the pinned Lance 11.0.0 implementation at ab6b5bbe46009ed78746b444df8db59a8bc5d842, alongside the full relevant upstream guides. Lance's row count subtracts deleted rows. Its branch checkout returns a version-pinned dataset. These support the count and snapshot assumptions. The count is still a metadata-based check, not a full payload comparison.
Tradeoffs and liability
Matching branch counts and using two controls improves the comparison. Each phase starts with a new handle. They do not clear OS caches, storage caches, or shared process resources. Service time includes engine queue waits. The ratio is a useful indicator, not a direct measurement of lock wait.
Peak RSS covers the whole child process. Fixture setup or the first control can hide a lower batch peak. The two RSS readings cannot establish memory per merge. The benchmark also bypasses HTTP admission and policy. Its 256-merge ceiling is an instrument limit, not a supported server capacity.
The result checks deliberately trade complete payload verification for lower verification cost. They catch wrong counts, missing sampled keys, and wrong receipts. They do not prove every vector value or every unsampled key. Keep that scope explicit when using these records.
The change adds 597 net lines. It adds maintenance for a fixture, verifier, and result format, but no new production state or publication mechanism. Sharing RunTarget, the watchdog, and attestation reduces duplicated harness code. My liability assessment is favorable: a checked-in instrument reduces reliance on scratch scripts and unsupported performance claims. After five similar additions, these shared helpers should remain the common owner. Separate scenario frameworks would reverse that benefit.
One existing limit carries into the new scenario: the watchdog starts after the storage probe and stops before teardown. It bounds the measured workflow, not every part of the child's lifetime.
Validation
All 12 benchmark_scenario_contract tests passed locally. I built the exact head with Rust 1.97.1, --locked, and failpoints in the development profile. Local runs with 1 and 4 merges passed. They checked 7 and 13 branches, and 3 and 6 receipts, respectively. A two-writer concurrent-writes run also passed after the helper extraction.
I temporarily compared each receipt against the wrong branch head. The benchmark failed with exit 101 and explicit verification failures. After restoring the source and rebuilding, the same small fixture passed. The checkout is clean.
The exact-head CI run passed workspace tests, formatting, and lint. I did not reproduce the remote-storage timing results or qualify release performance. The local runs check execution and verification, not the reported speedup.
| its clock starts before the release. | ||
|
|
||
| `batch.last_over_single` is the batch's last completion over the | ||
| controls' mean: 1.0 is perfect overlap and N is full serialization. |
There was a problem hiding this comment.
Optional: describe 1 and N as ideal reference points, assuming equal merge costs and negligible scheduling overhead. They are not thresholds that prove overlap or serialization. Cache effects, control drift, and resource contention can move the ratio below 1 or above N. My one-merge local run returned 0.917, with an after/before control ratio of 0.705. The existing drift and per-merge fields make those limits visible. Please state the assumption here and in the module comment.
| assert!(verification.contains("count_rows_branch")); | ||
| assert!(verification.contains("merged_parent_commit_id")); | ||
| assert!(verification.contains("receipts_checked")); | ||
| assert!(source.contains("concurrent-merges run invalid")); |
There was a problem hiding this comment.
Optional: supplement these source-text checks with a tiny executable check using the existing scenario fixture. These assertions check that names and messages remain present. They do not prove that a mismatch causes failure. During review, a temporary wrong-head comparison made the actual benchmark exit 101, and restoring it returned exit 0. Keeping that failure-path check would protect the evidence better than additional string assertions.
Main retired the Rust scenario harness into benchmarks/deferred/. This branch's concurrent-merges scenario and the run-target module it shares with concurrent writes follow it there as .disabled sources, the harness files this branch edited carry its bytes, sources.json records their checksums, and the active benchmarks README keeps main's text.
1 participant
Refs #643. Benchmark harness only; no engine change.
Adds a
concurrent-mergesscenario to the engine'sscenariosbench, the checked-in instrument #643 asks for: do merges into independent targets overlap, or queue?Shape. One repetition builds a
Chunkfixture onmainand N + 2 merge pairs. Each pair is a source and a target forked frommainthat insert--delta-rowsrows under their own keys, so every merge is a three-way merge without conflicts, never a fast-forward. Two pairs are controls, merged alone before and after the batch: every merge sees the same branch count, and the two controls show drift. Each measured phase opens a fresh handle outside its clock. The batch runs N tasks over clones of oneSession, released together by a barrier, and its clock starts before the release.Record.
batch.last_over_singleis the batch's last completion over the controls' mean: 1.0 is perfect overlap and N is full serialization.batch.per_mergeholds each merge's start, completion and service time. A peak-RSS pair brackets the batch. A fresh handle then requires exact row counts on every branch, reads each target's first and last merged key back, and checks every merge's publication receipt (#823): it must be the target's head commit, with the source's head as merged parent (verification.receipts_checkedcounts them). A merge error, an outcome other thanMerged, a missing receipt, or a mismatch fails the run; a deliberately wrong receipt comparison was confirmed to fail it. The record isclaim_grade: false: one batch on the host's clock is overlap evidence, not a throughput or latency claim.Shared plumbing.
concurrent-writesandconcurrent-mergesnow share the run root (local tempdir, or a probed unique S3 prefix with exit 78 on an unusable store), the watchdog and the attestation block, inscenarios/run_target.rs. That removes about 60 lines fromconcurrent_writes.rswithout changing its behavior or its record.Evidence. RustFS behind toxiproxy with 30 ms injected round trip,
--rows 256 --dims 8 --delta-rows 50, debug build, measured onc6f24757(the base #825 shares). A single merge alone takes about 4.9 s; the controls before and after agree within 2%.On main the batch's completions are staggered about 0.5 s apart, one merge after another. With #825 they cluster. Process peak RSS after the batch, over a 70 MB pre-batch peak: 81 MB at N = 8 on main; 99, 113 and 128 MB at N = 8, 16 and 32 with #825, since overlapping merges hold memory at the same time. These merges carry 50 rows each, so per-merge memory on real payloads will be larger. A scratch instrument with a different fixture measured 1.23 / 1.86 / 2.89 on main and 1.13 / 1.23 / 1.27 with #825, the same picture.
On a local filesystem a merge of this size takes about 30 ms, so the queueing hides; the README section says so and points at a latency-injected S3-compatible target.
Not measured here. Queue wait inside the engine (time at gates versus working) needs engine-side instrumentation; per-merge service time and completion offsets are what this records.
Contract.
benchmark_scenario_contract.rsnow reads the exit-78 refusal and exit-75 watchdog fromrun_target.rsand requiresconcurrent-writesto use it; a newconcurrent_merges_is_one_shot_labeled_and_verifiedpins the scenario's labeling, the barrier release with the clock started before it, fresh-handle verification with receipts, and registration.Checks. The rebased branch builds, and both scenarios run and verify locally on it.
cargo clippy -p omnigraph-engine --benches -- -D warnings -W clippy::dbg_macro,cargo fmt --check,typos,check-docs.pyandcheck-agents-md.share clean. No changelog fragment: the harness is internal.