Repository navigation
Conversation
…lock Every capture on a branch other than the handle's bound one goes through the single-entry merge_authority_cache, whose mutex was held across a miss's open_coordinator_for_branch, a full __manifest open and scan. Since the shared schema gate lets merges on different branches run together, they evicted each other's entry and then reopened one at a time under that mutex, which was most of their remaining serialization. A miss now opens with the mutex released and installs the entry after; the capture still reads the entry under the returned guard, the cache keeps one entry, and two misses on one branch open it twice. Measured with N merges into N independent targets on RustFS at +30 ms per round trip, the last completion over one merge alone goes from 1.23/1.86/2.89 to 1.13/1.23/1.27 for N = 2/4/8.
|
The checked-in instrument in #835 (
Every run verified exact row counts on every branch, merged keys read back, and each merge's publication receipt against its target's head. Memory is the cost of the overlap rather than of the cache: process peak RSS after the batch, over a 70 MB pre-batch peak, is 81 MB at N = 8 on |
ragnorc
left a comment
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: approve. I found no blocking correctness defect in 76fdd1dc5c64f253afa2e9e5785af691887624b1. This is a COMMENT review because the authenticated account is the PR author. The two inline comments are optional improvements.
What this PR does, in plain words
Several users or agents can merge separate branches through one server handle. Each merge first reads the branch's saved graph state. Previously, a cache miss held one shared lock during that storage read. Other merges waited, even when they used different branches. This PR releases the lock during the read, then takes it again to install and capture the result. It removes that unnecessary wait. It does not reduce the work inside each merge or change what a successful merge commits.
Contract and correctness
The useful workload is concurrent merges with disjoint source and target branches, especially when object storage has noticeable latency. Sharing either branch still serializes the merges. The existing branch gates enforce this scope. The deployment boundary remains one mutation-capable writer process unless external ownership provides equivalent protection.
I checked these safety conditions in the actual code:
- Each caller reads its selected coordinator under the returned cache guard. Another opener cannot replace the entry during that capture.
- A coordinator loads table state and lineage from the same pinned manifest. Branch identity surrounds the Lance checkout with checks. See coordinator construction and identity capture.
- Cache replacement can discard useful work. It cannot authorize a commit. The publisher selection checks the branch, lifetime, graph head and manifest version. A mismatch causes a fresh open.
- Every publication attempt reads fresh state and checks the expected graph head. One manifest publication still makes the graph change visible.
I checked Lance 11.0.0 at its packaged source commit ab6b5bbe46009ed78746b444df8db59a8bc5d842. Its branch checkout returns a separate dataset with a pinned manifest. Its strict overwrite path disables conflict rebasing when retries are zero. OmniGraph still uses that publication boundary. I also read the relevant full Lance format, versioning, branch, storage, read/write and performance guides.
Tradeoffs and long-term liability
The change trades waiting for concurrent work. More opens can run together, use temporary memory, and compete for storage bandwidth. The retained cache still has one entry. That does not imply unchanged peak memory. Cache replacement can also cause another open before publication. Cache-hit probes and refreshes still hold the lock.
The server already limits admitted writes to 64 globally and 16 per actor by default. However, merge admission estimates only 256 input bytes. These limits do not budget the merge's working memory. Embedded callers must bound their own concurrent work. Larger histories and bursty workloads therefore still need resource measurements.
My assessment is lower coordination liability with a small increase in concurrency reasoning and resource pressure. The diff adds 15 net lines, but adds no public API, persistent state, queue, cache map, or new authority. It fixes the lock scope that causes the wait. Five similar changes should retain this ownership pattern and explicit resource limits. They should not create five separate coordination mechanisms.
Evidence and limits
Local checks used the exact head, Rust 1.97.1, --locked, and --features failpoints. All 212 tests passed across branching, failpoints, merge_cost, merge_fast_forward, merge_truth_table, and warm_read_cost. Two subprocess helper entries remain ignored as standalone tests. I made no source edits.
The exact-head CI run passed workspace, RustFS, Azurite, formatting and lint checks. The DST run and GQT run also passed. Those are CI results, not local cloud tests. I did not reproduce the reported latency or RSS measurements. The current tests support correctness, but do not directly prove overlap during a cold coordinator open. The inline test suggestion targets that gap.
| behind each other. A merge that misses the handle's single-entry authority | ||
| cache now reopens that branch's coordinator without holding the cache's | ||
| lock, so concurrent merges into different targets overlap; merges into the | ||
| same target still serialize. |
There was a problem hiding this comment.
Optional: narrow the overlap claim to merges whose source and target branch sets are disjoint. branch_merge_impl takes exclusive gates for both branches (exec/merge.rs:5324–5328). Thus source → target_a and source → target_b still serialize, despite different targets. Also, cache-hit probes and refreshes still hold the cache lock. Suggested wording: ‘A cache miss no longer holds the shared cache lock during a coordinator open. This permits overlap between merges with disjoint source and target branches.’
ragnorc
left a comment
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: approve 3f4a7a05b0991584d01065d5fde290d40fdfb900. I found no new blocking correctness defect after the merges from main. This is a COMMENT review because the authenticated account owns the PR.
This PR lets independent branch merges spend less time waiting for one shared cache lock. A cache miss reads a branch's saved graph state. The code now releases the lock during that storage read, then takes it again to install and capture the result. The useful workload is several merges through one handle, with disjoint source and target branches and noticeable storage latency. The change does not reduce each merge's storage work.
The three merges since the previous review have no manual conflict resolutions. Against current main, the PR still changes only the cache helper and its comments, plus the release note. I traced the helper's current callers and the new history-cache and merge-input code brought in from main.
The design still preserves the required contract:
- Selection and capture use the returned mutex guard. Another opener can replace the cache only after that capture ends. Duplicate opens or a later replacement can waste work, but do not mix two coordinators in one capture. The same helper covers merge inputs, branch-control source capture, and non-bound authority capture.
- Branch opening checks native identity around checkout. Captured lineage copies the head and buffer from that coordinator. The shared history cache supplies immutable settled records; it does not supply the branch's current head.
- Merge admission still takes both branch gates. Preparation pins its inputs and checks their freshness. The publisher reloads authority and checks the expected head on every attempt. Cache reuse must match branch, lifetime, head, and manifest version; otherwise it opens a fresh coordinator. One manifest publication remains the visibility boundary.
This addresses the cause at the shared helper: storage I/O need not hold the lock that protects cache selection. It adds no public API, persistent state, queue, or second authority. I assess it as lower coordination liability, with more concurrent resource use to account for. Five similar changes should keep the existing ownership and publication rules rather than add separate coordination mechanisms.
The tradeoff is waiting versus concurrent work. The retained cache has one entry, but several in-flight opens can consume more temporary memory and storage bandwidth. A replacement can also cause a fresh open at publication. Hit probes and refreshes still hold the lock. The one-writer-process deployment boundary remains. Server admission limits operations; it does not prove a bound on total engine memory. Embedded callers must bound their own concurrency.
Local validation on the exact head passed 224 tests, with two subprocess helper entries ignored as standalone tests:
RUSTUP_TOOLCHAIN=1.97.1 CARGO_TARGET_DIR=/tmp/review-928/target \
RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable' \
cargo test --locked -p omnigraph-engine --features failpoints \
--test branching --test failpoints --test merge_cost \
--test merge_fast_forward --test merge_truth_table \
--test merge_projection_cache --test warm_read_cost -j3Documentation checks passed for 218 Markdown files. Instruction links, formatting, and diff whitespace checks passed. No source or test edits remain. I read the relevant upstream guides and checked Lance 11.0.0 checkout, refs, and strict overwrite code. The crate archive matches Cargo.lock; those installed source files match the archive at packaged commit ab6b5bbe46009ed78746b444df8db59a8bc5d842.
Main CI, GQT, and DST passed. Workspace CI checked out a6e711a3ec9ded9aaed632682bff5b54c4ccc601; its tree matches the reviewed head exactly (aa8265216ff82839447cb16d44c7671197ea5d14). RustFS and Azurite success is CI evidence, not a local cloud test.
The earlier optional wording correction and controlled cold-open test remain applicable. I have not duplicated those comments. GQT rows and errors do not distinguish this scheduling change, and no existing seam pauses this coordinator open. The existing Rust merge owner is the right place for that mechanism check. The focused suite supports correctness, but it is not a before/after proof of cold-open overlap. I did not reproduce the latency or peak-memory measurements, and I do not claim a new performance result.
1 participant
Refs #643.
What & why
Every capture on a branch other than the handle's bound one (merge preparation, branch-source capture) goes through
merge_authority_cache, a single-entry cache behind onetokio::sync::Mutex. On a miss,validated_cached_coordinatorcalledopen_coordinator_for_branch, a full__manifestopen and scan of about 16 requests (#813's figure), while holding that mutex.Since #783, merges on different branches share the schema gate and run together. They evict each other's entry and then reopen one at a time under this mutex. After #783 that was most of the serialization left between independent merges, and the field's comment still justified the mutex with "the schema serial queue already serializes merges … at capture time".
A miss now opens the coordinator with the mutex released, then re-locks and installs the entry. What does not change:
Two captures that miss on the same branch open it twice. The entry is a hint, and a non-bound publisher that finds a different branch's view in the slot already falls back to a fresh open.
Measured
N concurrent merges into N independent, diverged targets, on RustFS through toxiproxy at +30 ms per round trip, release builds. The value is the last completion over a single merge on a graph with the same branch count:
b14c22c5)main(c6f24757)With this PR, eight independent merges finish in 5.4 s against 4.2 s for one. The harness was a local ignored test, posted in full in #643. The measurement criterion of #643 would be best served by a checked-in
concurrent-mergesscenario in the bench harness, as a follow-up.Local verification
failpoints(121),branching(48),merge_truth_table,merge_fast_forward(20),warm_read_cost(17),branch_control_cost,merge_cost(4),consistency(23),writes(47) andschema_apply(30) owners pass, with--features failpoints.crates/omnigraph-dst, with the cost golden unchanged.cargo clippy -p omnigraph-engine --all-targets --features failpoints -- -D warningsandcargo fmt --checkare clean.Release note:
changelog.d/merges-overlap-across-independent-targets.performance.md(#833). The branch merges currentmaincleanly; on the merged treefailpoints,branching,merge_cost,merge_fast_forward,merge_truth_table,merge_cleanup,warm_read_cost,branch_control_costand workspace clippy pass. The checked-in instrument for the numbers above is #835.