report: publish the MongoDB CountScan campaign and its evidence - #20
Draft
carsontung666 wants to merge 26 commits into
Draft
report: publish the MongoDB CountScan campaign and its evidence#20carsontung666 wants to merge 26 commits into
carsontung666 wants to merge 26 commits into
Conversation
carsontung666
changed the title
Freezes the pre-registered protocol for a three-arm comparison of a classic CountScan change, before any measurement of these arms is taken. Arms are rebuilt from one pinned base in a single worktree with a byte-identical benchmark harness overlaid, so only six production files differ: A 0561c098b99a pinned upstream base B 4109dcc31ff6 an earlier heavyweight implementation, since rejected C 90814b83d3e5 the minimal candidate B is a strict mechanistic superset of C, so C/B is registered as a noninferiority comparison reported as a complexity trade-off; it deliberately does not veto adoption, which is decided by C/A, the controls and CPU non-regression on C/A. CPU time is not gated on C/B because at this block count it resolves to roughly plus or minus 1.5 percent, which cannot separate two implementations expected to differ by about one percent. Five workloads over 30 blocks give 450 fresh processes: three indexed-count endpoints, a point-query control, and a control whose plan is COUNT -> FETCH -> IXSCAN so the optimization cannot fire. The last is the only workload where a regression could hide, and its band is two-sided because on a path neither arm touches an apparent improvement is as diagnostic as a regression. The attested build records C1 == C2 byte-identically with A and B built in between, per-arm compiler, effective flags and bazelrc digests, and a campaign-size smoke per arm that pins the plan shape and iteration count. Also corrects two statements in the report: a per-key profile from a scalar count was worded so it could be read as characterising subtree retrieval, where the profile in fact inverts; and the short-read arms pin an index hint on both sides, which keeps them like-for-like but suppresses the server's point-query fast paths, so the baseline is not the fastest path available.
Binds attempt 001 to the exact frozen campaign.json SHA-256 and to every protocol and source artifact hash, and anchors it to the commit that froze them, which was pushed before any measurement of these arms. Discloses what was observed before the freeze: per-arm instruction levels from the build smokes, which calibrated the iteration rules. No ratio between arms was computed. The ledger is append-only and hash-chained; a locally held file cannot prove its own completeness, so the anchor commit is the evidence to check.
A test in the frozen protocol asserted that campaign.json was still an unexecutable draft. That became false the moment the campaign was legitimately frozen, so the suite failed on a transient state rather than on an invariant. The test now pins the two invariants that matter -- a draft cannot execute, and the frozen article validates strictly -- against synthetic states instead of whichever the file happens to be. Fixing a protocol artifact changes its hash and therefore the campaign hash that attempt 001 was bound to. Attempt 001 is recorded as superseded before execution rather than rewritten: no benchmark process was run and no measurement was taken for it, and leaving the sequence in place keeps it auditable. Attempt 002 is pre-registered against the new campaign hash and anchored to the pushed commit. Suite is 46/46 and execution-ready validation passes.
Attempt 002 aborted on its first process. The analyzer asserted that every iteration row carries repetitions == 5, but google-benchmark reports that field only on aggregate rows; iteration rows carry 0. The synthetic test fixture had been written from the same wrong assumption, so the suite stayed green while the assertion could never hold against a real run. The repetition count is still pinned, by the exact repetition_index sequence and by the aggregate rows' own iterations field, so nothing is checked less tightly. The fixture now emits what google-benchmark actually emits. Attempts 001 and 002 are retained in the ledger. Neither completed a single benchmark process and neither produced any measurement; deleting an unfavourable or failed attempt is precisely what the ledger exists to prevent.
The three-arm campaign ran to completion: 450 processes, 30 blocks, all five workloads. The candidate retires 4.600%, 2.055% and 3.680% fewer instructions than the pinned base on the scalar, multikey and compound-wildcard count endpoints, favourably in every block, which is 118.0, 116.1 and 117.1 instructions per counted document. The pre-registered adoption gate did not pass. Both controls fell outside their bands, and the report leads with that rather than burying it. No CPU-time or wall-time result is claimed from the experiment, although the pre-registered CPU gate passed for the candidate: the point-query control shows the instrument reports a few percent of apparent improvement on plans where the instruction count does not change, so those numbers are discarded. The non-firing-count control produced the finding that matters most, and it is not about the candidate: the rejected heavyweight implementation retires 24 more instructions per fetched document on a plan the optimization cannot reach. Commits the artifacts that were previously only on the authoring host, so the results section is reproducible from a clone: the 450 raw process JSONs and logs, summary.json, the run record, and the correctness and non-intrusion evidence under validation/. Adds analyze_controls_posthoc.py, which reproduces the post-hoc control analysis the report quotes and states plainly that it is not pre-registered and cannot change the gate. Three provenance disclosures the report now carries rather than leaves to a reader to discover: the build smokes had already measured every workload at campaign size before the protocol was frozen, so the campaign establishes the intervals rather than the discovery; the attempt-003 pre-registration cites the wrong anchor commit, corrected in anchor_correction.json; and the campaign measured 90814b83d3e5 while the commit under review is 90781b36b2, with the diff and a re-smoke under validation/review_fix_equivalence/ showing the difference is smaller than arm C's own process-to-process spread.
carsontung666
changed the title
Narrowing express eligibility so that only batchSize 0 disqualifies a query removes 37.8% of the retired instructions on a unique-field point query that sets batchSize (171,098 to 106,490), order-independent, with the no-batchSize negative control flat at 1.0007. The patched batchSize path lands on the same cost as the no-batchSize express path, which is the mechanism. It is not proposed as a change. profile_find.js uses batchSize deliberately, with the comment "Use batchSize to avoid express path", and then asserts planSummary, queryHash and planCacheKey; the exclusion is a documented escape hatch rather than an oversight, and that test fails. The change also alters cursor lifecycle: where batchSize equals the number of matching documents, the reply now carries cursor id 0 instead of leaving a cursor for a getMore that returns nothing. Whether to give up that escape hatch, and what the profiler and slow-query log should report for the affected queries, is a query-team decision that local evidence cannot settle. Recorded with the parity captures, the benchmark output and both resmoke reports so it can be picked up rather than rediscovered.
A blank review of the published state found three things worth fixing. The abstract and conclusion described the optimization as firing "only on single-solution plans", which reads as a scope limit. It is not one: QueryPlanner::plan returns a count scan as the single solution as soon as one exists, so every classic COUNT_SCAN plan qualifies and none is multi-planned or cached. The section body already said this; the summaries contradicted it. The claim that reverting the production files gives 13 PASS / 1 FAIL had no artifact behind it, in a bundle that ships 450 raw JSONs for everything else. It is the one number that proves the new test detects the change. Both dbtest runs on the reviewed commit and the base-revert run are now retained, along with the build log for the benchmark the PR ships. The equivalence smoke is one process per endpoint against a 30-process mean, and the comparison band is a range rather than a dispersion measure. Both are now stated: on the multikey endpoint the difference is about 2.3 per-process standard deviations, or +0.54 instructions per counted document against an effect of 116. Also: the setter now declines rather than asserting when the scan has already been worked. Two of the four CountStage construction sites wrap already-built rejected plan roots for explain, and SERVER-118659 proposes bringing count scans under cost-based ranking, which is what would make that reachable. Declining means landing it costs this optimization rather than a user's explain. The prior-art row claiming the candidate reuses trackWork was stale — that was the rejected arm's design.
Entity fetch. IndexBoundsBuilder deep-copies the whole $in list every time it builds bounds, because BSONElement::Obj() never yields an owned buffer and so InListData::isBSONOwned() is false for every $in parsed from a command. Sharing the already-owned filter instead costs a refcount. Measured 1.01%, 1.14% and 1.80% fewer retired instructions for list lengths 10, 100 and 1000, growing with length as an O(n) copy predicts. A negative control on a query with no $in bounds binary-layout drift at 0.4%, well below the 2,546 to 15,011 instruction differences measured. Records a correction: an earlier run gave 2.11/2.02/2.13% and those numbers were published first. They were taken under build load. Re-running both binaries back to back reproduces the earlier binary's figure to within one instruction, so the difference was the machine. The lower numbers stand. A blank review found a real regression this change introduced and it is fixed here rather than argued away: sharing swaps a bounded copy for an unbounded pin, and an InListData reaches the SBE plan cache, where single-solution entries are pinned and the budget estimator does not count inLists. A small list inside a large filter would pin the filter indefinitely, so sharing is declined when the filter is more than a kilobyte larger than the list.
Child expansion and subtree retrieval. IndexScan decodes every index key to BSON, but on an IXSCAN -> FETCH plan that BSON has no consumer: WorkingSetCommon::fetch clears it unread whenever no yield occurred. An earlier audit dismissed this because the key is needed by the yield-time consistency check. That reasoning does not hold. The check re-encodes the stored BSON back into a key_string::Value to look up in a KeyStringSet, so the current path is KeyString -> BSON -> KeyString and the storage layer already had the KeyString. CountScan already passes KeyInclusion::kExclude, and there is a TODO in working_set_common.cpp asking for exactly this opt-out. The A/B needs no new code: sorted_data_interface_bm already registers the same cursor advance under both settings. Not materializing the key roughly halves the cost of advancing the cursor - 47.6% less CPU on a non-unique index, 52.9% on a unique one. That is a ceiling on the per-key component, not a query-level projection, and it is CPU time rather than retired instructions. Recorded with the implementation design so the work starts from a measured number. Not implemented: its correctness rests on deciding when the flag is safe, and keyData has seven consumers across 73 references, where setting it wrongly is a silent wrong-results bug rather than a crash.
…y off Child expansion and subtree retrieval. FetchStage now tells a direct IXSCAN child that nothing above will read its keys, mirroring how CountStage tells CountScan; IndexScan applies it only when it also has no bounds checker, filter or key metadata, so the condition is decided locally rather than by a planner flag. It then keeps the cursor's KeyString and the post-yield consistency check compares it directly instead of re-encoding BSON. Correctness is confirmed. The opt-in fires on 217 of 217 IXSCAN -> FETCH scans and 0 of 127 covered-projection scans, and 42 core jstest entries pass with a yield forced on every iteration, which is what makes the rewritten check run. The effect is not established and is reported as such. Two pairs of binaries built from identical production source measure 2.01% fewer instructions and 0.63% more; pair one reproduces exactly on re-run, so the builds genuinely disagree rather than the runs being noisy. The mechanism explains it. The storage layer's kExclude wins by producing nothing - measured here at 47.6% and 52.9% off cursor advance, using an A/B that already exists in sorted_data_interface_bm and needed no new code. But a FETCH still needs per-key data across work() calls, so this retains a key_string::Value from getValueCopy(), which allocates. It trades a BSON build for a KeyString copy rather than removing work, and the storage ceiling does not transfer. No pull request was opened, because a change with no demonstrated win should not be proposed. The branch is retained because its correctness scaffolding is what any future attempt would need.
…tions Records what was tried and precisely why it failed, so the next attempt does not repeat it: the storage-layer kExclude ceiling does not transfer to a FETCH plan because the consistency check forces per-key state to be retained, and the express batchSize exclusion is an escape hatch a core test depends on. Also records the measurement discipline the hard way. Build-to-build variation on identical production source reached 2.6 percentage points, so a single base-vs-patched pair proves nothing at the few-percent level; every claim needs a control benchmark the change cannot fire on, run on the same binaries, and the pair re-run to confirm. Two sets of published figures were wrong for exactly these reasons. Points at the covered-projection path as the most promising untried lead, since the projection consumes the key immediately and retains nothing, which is what made the FETCH case a dead end.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
bench/db/report/report.tex/report.pdfas the canonical database reportReview decisions
CountStage -> CountScan: narrow source candidate on a pinned master snapshot, adoption gate did not pass — see belowNo MongoDB optimization in the report is currently integrated into ConDB's public storage path.
MongoDB source candidate
0561c098b99a90781b36b2; the campaign measured its parent90814b83d3e5, retained as brancharchive/countscan-measured-90814b83, andvalidation/review_fix_equivalence/shows the difference is smaller than arm C’s own process-to-process spreadcarsontung666/mongo:agent/condb-query-hotpath, draft review carsontung666/mongo#1CountStage::doWorkreads only its child'sStageStateand immediately frees anyWorkingSetMemberit receives, so a directCountScanchild builds and tears one down per advanced key with no consumer. The candidate lets such a child skip it: one boolean, one branch, and a private setter reachable only by aCountStageparent. Deduplication, memory accounting and its limit,keysExamined, yield handling and save/restore all run above the skipped step.It fires only when
CountStage's child is a bareCountScan, i.e. the single-solution plan case. Multi-planned and plan-cached counts have aMultiPlanStageorCachedPlanStagein between and are unaffected.An earlier, much larger implementation of the same idea — a
trackWorktemplate on the base class of every classic stage, a test-onlyfriendin a production header, a second entry point, and a devirtualization — was rejected in review and is retained here as a measured arm rather than deleted.What the campaign found
450 fresh processes: 30 blocks x 5 workloads x 3 arms, stratified over all six arm orders, one CPU per process under the performance governor, retired user-space benchmark-thread instructions as the pre-registered primary metric.
118.0, 116.1 and 117.1 instructions per counted document. The multikey row is the one that establishes the normalisation — it advances over twice as many keys as it counts documents and the saving still lands per counted document.
The pre-registered adoption gate did not pass, and the report says so in its abstract, its first Results sentence and its conclusion. Both control workloads fell outside their bands: the point-query control showed a ~2.3% CPU-time offset that decomposes onto the binaries rather than execution order, on a plan the change cannot execute; and the non-firing-count control had a ±0.2% band that was unattainable at 30 blocks against its measured 1.57% dispersion. No CPU-time or wall-time result is claimed from this experiment, even though the pre-registered CPU gate passed for the candidate — the same instrument reports apparent CPU improvements on two plans where the instruction count is unchanged, so those numbers are discarded rather than published.
The second control produced the finding that matters most, and it is not about the candidate: the rejected earlier implementation retires 24 more instructions per fetched document on a plan where the optimization cannot fire, while the candidate is indistinguishable from the base there. That is the empirical form of the source reviewers' objection to touching every classic stage's base class.
Evidence bundle
bench/db/report/evidence/mongodb_master_countscan_20260805_4109dcc31ff6/Three disclosures are carried in the bundle and in the report rather than left for a reader to find:
anchor_correction.jsongives the correct commit, the check that distinguishes them, and an account of a mistake made while filing the correction.validation/was run onac20554f, which differs from the candidate by oneclang-formatinclude-order line, and was not re-run afterwards.The attempt ledger retains all three attempts, including the two that produced no measurement.
Not claimed
Retired instructions are not latency. Every plan measured is hint-forced. Single-threaded, in-process, warm-cache, one host. Nothing here is a 7.0.34, SBE, sharded, ConDB-endpoint or production-latency result, and nothing here is upstream-ready: there is no MongoDB ticket, no contributor-agreement path, and no Evergreen run, so the fork PR stays a draft.