Skip to content

test(bench): add unattached branch root-write scenarios - #961

Merged
azimafroozeh merged 4 commits into
ModernRelay:mainfrom
azimafroozeh:unattached-branch-bench-cost
Oct 12, 2026
Merged

azimafroozeh merged 4 commits into
ModernRelay:mainfrom
azimafroozeh:unattached-branch-bench-cost

Conversation

@azimafroozeh

Copy link
Copy Markdown
Collaborator

What & why

This PR adds three unattached root-write scenarios to the benchmark catalog.

  • Tiny insert: shares the existing tiny-mutate fixture.
  • Published history: writes a new root after 32 published updates.
  • Retained chain: writes a new root beside one retained root and 31 children.

Each scenario verifies snapshot contents, preserves main, and drops the measured snapshot outside measurement.

Backing issue / RFC

  • Benchmark follow-up to PR 956, related to Unattached graph branches. The RFC remains draft; cost acquisition and chained-write attribution remain pending.

Checklist

  • Focused: root-write benchmark inputs and correctness coverage.
  • Tests: all three scenarios execute; the retained-chain owner checks 32 snapshots, exact ancestry, and unchanged inventory after cleanup.
  • Public docs: updated benchmarks/README.md with scenarios and measurement limits.
  • Release note: added changelog.d/unattached-branch-benchmarks.added.md.
  • Invariants: reviewed against docs/dev/invariants.md; no Hard Invariant weakened or deny-list item hit. Nearest invariant: evidence matches the boundary.

Local verification

  • env RUST_MIN_STACK=16777216 cargo test -p omnigraph-bench --locked --lib gqt_tests:: -- --test-threads=1: 103 passed. The default-stack run aborted in unchanged bounded_hot_table_idle_base.
  • cargo run -q -p omnigraph-vocabulary-guard -- check --surface rust-string --base c92b267d6153f4342e4d7c03c26b8711f7a79ab1 --inventory tools/omnigraph-vocabulary-guard/graph-vocabulary-inventory.tsv --openapi openapi.json: failed on the existing merge-base inventory. The changed Rust file has identical base/current scans with zero observations; the existing CI audit is disabled.
  • Release benchmark acquisition: not run. This PR supplies scenario inputs and makes no performance claim.

Notes for reviewers

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@ragnorc ragnorc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve the benchmark change at d7f467fdbd23c4a3dc144713a973e1981a1d384d. I found no blocking code defect. The separate DST failure below still needs to be resolved before merge.

This PR supplies repeatable inputs for measuring an unattached root write. It adds a tiny insert, an update after 32 published updates, and a new root beside 32 retained unattached snapshots. Each workload checks the new snapshot, checks that main keeps its rows, and drops the measured snapshot after the timed operation. The history workload also checks the snapshot after reopening. These inputs let later runs compare costs; this PR supplies no speedup result.

The evidence matches the stated boundary:

  • The selected operation hooks enclose the engine mutation. Assertions, snapshot reads, restart, and drop run afterward. A failed verification rejects the sample. The existing prefix rules also prevent a prerequisite write from being hidden before the selected operation.
  • The retained-chain check verifies all 32 live snapshots, one root, exact parent links, and consecutive generations. It compares the full chain and published inventory before and after the workload. Thus 32 independent roots, an advanced main, or loss of a retained child cannot satisfy this fixture check.
  • The tiny and published-history cases reuse their ordinary-write fixtures. The retained-chain case is a different storage-history treatment. It measures a new root beside that chain, not an extension of its tip. It cannot alone isolate the cost of publication or establish chained-write throughput. The README states the root-only and merge-only attribution limits.

I traced the mutation through unattached staging, the detached overwrite, and snapshot drop. I checked the complete relevant Lance guides and format documentation against the pinned implementation: Lance 11.0.0, source ab6b5bbe46009ed78746b444df8db59a8bc5d842. Its detached commit path writes a detached manifest; native tags retain its address. Dropping a snapshot removes its reference. It does not establish physical space reclamation. The new cases make no cleanup-cost claim.

The maintenance tradeoff is favorable. The assertions reduce the risk that fixture drift silently changes a measured treatment. The PR adds fixture and assertion code, but no production state, public API, storage format, or timing abstraction. Sharing fixtures and the history workload limits drift. The explicit 32-step recipe costs maintenance, but it fits the existing rule that commit bindings cannot appear in loops. Five similar additions can still use the same catalog and runner. There is no need to expand the GQT language for this change.

I also surveyed the other open PRs. The cache work in #818/#825 and group-commit direction in #785 may change these costs. The existing source/engine identity keeps those runs distinct; these root-write cases do not qualify those separate execution paths. Current upstream Lance 13.0.0 and 14.0.0 beta are future test targets, not versions validated here. This PR adds no Lance adapter to carry through an upgrade.

Validation:

  • Local, clean isolated checkout at the exact head above: git diff --check d334dc3c0510481a2484ad0ad0b65d1b23c99d8c HEAD passed.
  • Local command: RUSTUP_TOOLCHAIN=1.97.1 CARGO_TARGET_DIR=/tmp/review-928/target RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable' RUST_MIN_STACK=16777216 RUSTC_WRAPPER= cargo test -p omnigraph-bench --locked --lib gqt_tests:: -- --test-threads=1 — 103 passed, 0 failed, 0 ignored, 306 filtered out; test execution took 46.60 seconds. This includes both new history tests and the tiny mutation case in the receipt test. The build reused the existing target cache.
  • This is new benchmark coverage, not a behavior fix. The comparison base is d334dc3c0510481a2484ad0ad0b65d1b23c99d8c; the reviewed head is above. A before/after failing bug regression is not applicable. I made no temporary source or test edits.
  • CI and GQT passed. CI checked merge commit 5c9a358232519fe8d42307066269679318ab7681, whose tree matches this head (b8204aa49e1b1d7f58ddbbfc317217374b77b777).
  • DST pinned suite failed: its scenarios executable reported 50 passed, 1 failed, 28 ignored. dst_schema_apply_racing_writers_first_contact, seed 24301, failed its schedule-coverage assertion at scenarios.rs:4826: no schema apply landed between two writer commits. This PR changes neither that test nor production code. That observation does not prove the failure is harmless; resolve it or obtain a passing rerun before merge.

Local checks exercise the benchmark inputs and adapter on small tables with fixed payloads. I did not acquire release performance records, test these cases through HTTP or cloud storage, or validate concurrent, large-payload, or many-table workloads. No optional code changes requested.

@azimafroozeh
azimafroozeh added this pull request to the merge queue Oct 12, 2026
Merged via the queue into ModernRelay:main with commit f53bf4f Oct 12, 2026
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants