perf(engine): hydrate only global semantic winners - #85
Conversation
Cold unfiltered kNN decoded complete stored sources for every scored row even though ranking only needed IDs, sequence numbers, vectors, and chunk vectors. On a 10,000-row, eight-segment query this made source reconstruction scale with the corpus instead of the requested top-k. The first selective implementation then reused the segment build mutex for selected-row decoding; because those decodes publish no shared resident value, c8 callers convoyed behind work that each caller still had to repeat. Decode a typed cold projection containing document identity, sequence number, vector, and optional chunk vectors. Rank exactly as before, retain the winning segment/ordinal/sequence identities, and reconstruct complete `_source` only for the global winners. Preserve the compatibility fallback for non-ZBS2, unsupported dependency shapes, missing vector columns, existing full-source caches, filtered queries, SQ8, and other shapes that need sources while scoring. Keep projection construction single-flight because it publishes a shared cache entry, but decode selected rows independently: they have no shared publication and must not take the projection/full-source segment build lock. The c8 regression test delays eight same-segment row decoders and requires actual overlap plus eight exact identity/source results. Make correctness fail closed. Selected rows must match the ranked ID and sequence number; missing or mismatched winners are errors. Snapshot-version, tombstone, update/delete, merge-retirement, restart, and cache-race tests cover the boundary between ranking and hydration. Passage selection and complete source vectors remain exact. Give projection and row workers cooperative deadline/drop cancellation. Use one request-wide post-ranking hydration grace, never expose null placeholder hits, and mark responses timed out when the original deadline elapsed even if bounded hydration completed inside the grace. Projection-stage cancellation returns an integral partial result, and fallback hydration obeys the same single-grace rule. Charge retained projection capacity to the shared segment hydration budget, including the compound key, IDs, sequence numbers, vectors, and chunk-vector capacity. Refuse publication and use the compatibility path when admission fails; retire and uncharge entries with segment lifecycle cleanup. Align the ingest-memory and cache-budget accounting constants used by this resident shape. The first diagnostic binary with the selected-row lock exposed a c8 convoy: its median B/A ratios were 1.638863 request latency, 2.175760 wave makespan, and 0.460016 throughput. Removing that non-publishing row lock changed the same v7b ratios to 0.461781, 0.411865, and 2.428588. The c1 v7b medians were 0.458700 latency, 0.465141 makespan, and 2.167564 throughput. These are DIAGNOSTIC_ONLY two-repetition ratios with an uncontrolled OS page cache, not a release verdict. Evidence: /workspace/.tmp/pr1-current-main-production-ab-v7b-evidence-20260731/results.json (sha256 1c0713465c51449f454ac1887741150470378a5049e282e41e3f5df41b099515) and /workspace/.tmp/pr1-current-main-production-ab-v6-evidence-20260731/results.json (sha256 696448de34979c5ef3001b11939c9c7b0f346b6ac59b99df97887fa303ef3772). Across v7b, all 576 primary responses and 64 same-clone restart responses returned HTTP 200 and matched ordered IDs, f32 score bits, complete source vector bits, and passages against the pinned exhaustive lexical oracle. The finance tree remained byte-identical and logical/allocated disk ratios stayed 1.0. Evidence root seal: /workspace/.tmp/pr1-current-main-production-ab-v7b-evidence-20260731/SEAL.sha256 (sha256 48211ab012a683a4eb1aea28622fab082deda8ffb71f82bf5d5b1c6d08d8ffec). The mandatory ES-YAML gate against the exact sealed v7b binary completed with 1360 passed, 0 failed, and 3 skipped. Evidence: /workspace/.tmp/pr1-conformance-v1/summary.json (sha256 0a86c22848f8ecc61e3ced5560468dfd1fbf929435027072d8fd6eff81a88a93). The sealed candidate is 53,930,648 bytes versus the 53,413,568-byte current-main baseline: +517,080 bytes (+0.968%). Binary identities: /workspace/.tmp/pr1-current-main-bench-binaries/candidate-no-row-lock/identity.json (sha256 633d34e0106d25638e56acdeb8af9e751e703c1a6b45a64e1ca34992c22da946) and /workspace/.tmp/pr1-current-main-bench-binaries/baseline/identity.json (sha256 799d35042af18a129d9e6e9d9fc4be4e119458d23783cac15b6bf6b314569909). Limitations: the performance campaign has two repetitions; external `/proc` RSS/PSS includes sampling jitter; OS page cache was uncontrolled; current main has neither prototype route counters nor benchmark-authoritative jemalloc telemetry. This change does not claim a production win, change filtered/SQ8 semantics, enable neural embeddings, or close the broader mixed read-under-write scorecard gap.
The PR release suite intermittently failed the timeout-side-effect assertion with a hydration count of one instead of zero. The counter was a crate-global test atomic even though Rust runs unrelated index fixtures in parallel. TEST_SERIAL only ordered this module's fixtures; a selected-row decoder belonging to another Index could still increment the shared value after this fixture reset it. Move the selected-row hydration counter behind cfg(test) onto Index and pass that Arc into ColdRowDecodeWorker. Assertions now observe only workers created by their own fixture. The production timeout, cancellation, ranking, hydration-grace, and fail-closed paths are unchanged, and the zero-hydration assertion remains strict. Before: GitHub Actions run 30656257769 failed at index.rs:27876 with left=1/right=0 while 264 sibling tests passed. After: the targeted release test passed after rebuild plus 20 repeated runs; all 14 selective hydration tests and all 265 xerj-engine release library tests passed. cargo fmt --all -- --check and release clippy for xerj-engine lib/tests with -D warnings also passed.
The release CI still observed one selected-row hydration after the per-index telemetry isolation. That hydration belonged to the fixture itself: the test used a 15 ms request deadline and a 20 ms scan delay, but the semantic executor intentionally grants one fixed 50 ms post-ranking window so exact winners can finish hydration while returning timed_out=true. Replace the scheduler-dependent sleep/checkpoint race with the existing ranking barrier. The test now pauses precisely after winners are ranked, forces the post-ranking timeout path, waits beyond the fixed grace, releases the barrier, and proves that no selected row is hydrated after that boundary. Production timeout and hydration behavior is unchanged. Validation: the focused test passed 10/10 repeated runs; cargo fmt --all -- --check, git diff --check, and scoped xerj-engine lib/test Clippy with warnings denied passed.
|
Reviewed this for inclusion in v1.0.0-rc.9. We're going to hold it for rc.10 rather than take it now — reasons below, all with file:line so they're actionable. Full disclosure on method: this was reviewed by three independent passes (correctness, concurrency/resource safety, release risk), with every finding sent to a separate verifier whose job was to refute it. 27 candidate findings, 20 refuted, 7 survived. Everything below is a survivor. Leading with the good news, because it's the part that matters most: the correctness pass found nothing. Nine candidate findings, all nine refuted. Ranking equivalence genuinely holds — The blocker: steady-state kNN gets slower, which inverts the PR's purposeThe eligibility gate at That hydration is not O(k). On main, the first kNN over a cold segment paid decompress + parse once and every query after was a DashMap lookup plus an Two things make this easy to miss rather than careless: a two-repetition A/B measures cold first touch, which is exactly the case where this doesn't show; and the PR's own test The repo already has the cheap variant — Two more that survived refutationPartial hydration drops winners by hash order, not by rank. Winners are grouped into Counter isolation is incomplete. 1e314af moved one counter onto On the flakiness, with a branch you're welcome to takeMeasured on One negative result worth having, because it cost us a cycle: the obvious fix does not work. Replacing the sleep with a rendezvous — block each decoder until N arrive — made it worse, 3 failures in 25, all reporting What does work is proving the property directly instead of observing it: have the test hold the segment build lock across a That work is on Genuinely nice change — the ranking equivalence surviving nine refutation attempts is not a common outcome. It's the cache interaction that needs another pass, and a benchmark that measures the second query rather than the first would have caught it. |
|
Do not merge untill I'll investigate |
|
Housekeeping, not a review comment. We've just added a Contributor License Agreement to the project (#93). Until now contributions here were covered only by Apache-2.0 §5's inbound=outbound clause, which gives no explicit patent grant — worth tightening for a project this size. Once #93 merges, a
That PR is the signature — it comes from your own account, so the commit history is the record. To be explicit about something: we did not add anyone to the signed list on their behalf, including you. That file asserts a person has signed, and that's not ours to assert for someone else — hence the ask rather than a quiet edit. Sorry for the extra step on work that's already in flight. Thanks for the contributions. |
|
Thank you for the detailed review. I reproduced the steady-state problem, and you are right: PR #85 should not merge in its current form. Doing the homework on the benchmark I have used... It indeed gives this speedup numbers but in specific env.. I ran a new release-mode benchmark specifically for the case the original evidence missed: 4,096 persisted documents with wide financial-report-shaped
So the tradeoff is unambiguous. The PR makes the first cold query 16.4% faster and uses 14.5% less point-in-time RSS in that phase, but by Q100 throughput is 56.3% lower and cumulative time is 2.289x worse. After reopen, Q10 throughput is 71.1% lower and cumulative time is 3.463x worse. The RSS column is checkpoint The cache state and code path confirm your diagnosis. Main reported all eight segments in This is consistent with the earlier sealed 10K result: that workload had a comparatively narrow source, where projection benefits outweighed repeated decoding in the measured cells. Without stage counters, I cannot assign the difference causally. That result was valid for its measured workload, but it was insufficient evidence for the broader steady-state claim in this PR. I also confirmed the other findings:
I have a focused correctness patch in validation that orders segment work by each segment's highest global rank, makes one selected-row hydration call per segment before any fallback, truncates partial results to the longest hydrated global prefix, moves the counters onto I do not think a winner-row cache is a sufficient repair. A test-only cache made an identical-winner sequence 7.16x faster, but a 100-query zero-overlap sequence had 0% cache hits and performed exactly the same 272 decodes, 418,527 encoded-row visits, and 16.413 MB of decompressed output. It overfits winner locality. The durable direction is independently compressed stored-row/column blocks plus a bounded decoded-block cache, so miss cost scales with touched blocks rather than an entire segment column. I will keep that storage redesign separate from the correctness patch and prove compatibility, disk size, cold/warm latency, memory, and exact query behavior before asking for another review. One tooling caveat: the authoritative numbers above are deliberately unprofiled release timings. The current engine-library harness cannot consume the server's pprof/heap endpoints, and the existing semantic phase trace lacks codec row/byte/cache counters. I will add those counters and run separate CPU-only and heap-only profiles before claiming stage-level percentages. That instrumentation gap does not affect the paired wall-time/RSS conclusion above. The audited evidence is retained with source, binary, launcher, raw-output, and host-ledger hashes. Thanks again for catching this. I am withdrawing the merge recommendation for the current PR and will rework it rather than argue with the measured result. |
Result
On the sealed 10,000-document exact semantic-search diagnostic, this change delivered 2.17x higher single-request throughput and 2.43x higher eight-request throughput, while reducing sampled query-serving peak process memory by 13.5x at c1 and 22.3x at c8. All 640 measured and restart responses preserved the exact ordered IDs, f32 score bits, complete source-vector bits, and winning passages expected by the pinned oracle.
This is exclusively a query-serving result over an already-built index. It does not measure or claim lower memory during PDF extraction, embedding generation, bulk indexing, flush, merge, or HNSW construction. It applies to eligible cold, unfiltered exact semantic scans and is not a claim that every XERJ workload uses 22x less memory. The campaign used two repetitions and an uncontrolled OS page cache, so the numbers are diagnostic evidence rather than a universal production claim.
Summary
This PR changes cold, unfiltered kNN search over typed ZBS2 stored segments so ranking decodes only the fields it needs—document ID, sequence number, vector, and optional chunk vectors—and reconstructs complete
_sourceonly for the final global top-k winners. It keeps exact ranking and response semantics, preserves compatibility fallbacks, adds cooperative timeout cancellation, accounts the new projection in the shared cache budget, and removes a selected-row lock that caused a same-segment concurrency convoy.User impact
Semantic search no longer has to reconstruct every document source merely to return a small top-k result. In the sealed 10,000-document diagnostic, the final no-row-lock candidate used 4.48% of baseline peak RSS and 4.36% of baseline peak PSS at c8 while returning exact oracle results for ordered IDs, f32 score bits, source-vector bits, and passages. The c8 candidate/baseline median ratios were 0.4618 for request latency, 0.4119 for wave makespan, and 2.4286 for throughput. These are
DIAGNOSTIC_ONLYtwo-repetition observations with uncontrolled OS page cache, not a production verdict. Local evidence:results.json(sha256:1c0713465c51449f454ac1887741150470378a5049e282e41e3f5df41b099515).Put plainly, the controlled diagnostic was a little over 2x faster:
This is a query-time optimization, not an indexing-speed optimization. It benefits eligible cold, unfiltered exact semantic scans where the caller asks for a small top-k from a much larger stored corpus.
How it works
The cold ZBS2 path decodes a typed projection keyed by segment, vector field, and optional chunk-vector field. The projection holds IDs, sequence numbers, vectors, and chunk vectors without reconstructing arbitrary source JSON. The existing exact scorer ranks those projected rows and retains each candidate’s segment, ordinal, and sequence identity. After global ranking, selected-row decoding reconstructs complete source objects for only the winning ordinals, verifies that each decoded ID and sequence number still matches the ranked identity, and applies those sources to the final hits.
Projection construction remains single-flight because it publishes a shared resident cache value. Selected-row decoding deliberately does not use that build lock: it publishes nothing shared, so serializing callers only creates a convoy before each caller repeats its own decode. The regression test delays eight same-segment selected-row decoders and requires concurrent activity plus exact identity and source equality.
The optimized path is gated conservatively. Non-ZBS2 segments, unsupported stored dependencies, missing vector columns, active full-source caches, filtered queries, SQ8, and other query shapes that require source material during scoring retain the compatibility behavior.
Safety and correctness
Winner hydration is fail closed: a missing source, ID mismatch, or sequence mismatch is an error rather than a partially populated hit. Tests cover updates and tombstones, snapshot identity across concurrent update/delete, segment retirement during projection publication, merge at the ranking/hydration boundary, cache races, restart, passage selection, and complete source-vector reconstruction.
Projection and row workers check both a cooperative cancellation flag and an absolute deadline. Dropping the async caller arms cancellation for the blocking decoder. A request gets one captured post-ranking hydration grace; it never emits null placeholder hits, and a response still reports the original deadline timeout when exact hydration finishes inside that grace. Projection timeout, selected-row timeout, natural deadline crossing, and compatibility fallback use the same integral partial-response rules.
Projection admission charges retained capacity for the compound key, IDs, sequence numbers, vectors, nested chunk-vector containers, and their buffers to
SegmentHydrationBudget::VectorProjection. A denied charge falls back without publication. Segment retirement removes and uncharges the entry under the existing lifecycle authority.The sealed v7b run checked every HTTP response against the pinned exhaustive lexical oracle: 576 primary responses and 64 unmeasured same-clone restart responses matched ordered IDs, f32 score bits, every source-vector bit, and passage fields. Final
finance/trees were byte-identical and logical and allocated disk ratios remained 1.0. Local evidence seal:SEAL.sha256(sha256:48211ab012a683a4eb1aea28622fab082deda8ffb71f82bf5d5b1c6d08d8ffec).Benchmarks
Ratios below are candidate/baseline. Lower is favorable for latency, makespan, RSS, and PSS; higher is favorable for throughput. Cold means a fresh process and empty XERJ query cache; OS page cache was uncontrolled.
Source: sealed local v7b reducer output
results.json(sha256:1c0713465c51449f454ac1887741150470378a5049e282e41e3f5df41b099515).The rejected first result and the convoy fix
The first selective-hydration diagnostic completed correctness closure but exposed a concurrency regression: its selected-row worker reused the segment build mutex even though row hydration did not publish a shared value. At c8, callers serialized on that mutex and then each performed the same necessary local decode, producing median B/A ratios of 1.638863 latency, 2.175760 wave makespan, and 0.460016 throughput. Rejected local v6 diagnostic
results.json(sha256:696448de34979c5ef3001b11939c9c7b0f346b6ac59b99df97887fa303ef3772).Removing the non-publishing selected-row lock changed the c8 medians to 0.461781 latency, 0.411865 makespan, and 2.428588 throughput. Relative to the rejected v6 ratios, that is -71.82% latency, -81.07% makespan, and +427.93% throughput. The peak RSS ratio moved from 0.039879 to 0.044828 and PSS from 0.040295 to 0.043580. These cross-campaign percentages are useful diagnosis, not a release claim.
Binary size
The sealed candidate binary is 53,930,648 bytes; the identically configured current-main baseline is 53,413,568 bytes. The delta is +517,080 bytes, or +0.968%. Local candidate identity
identity.json(sha256:633d34e0106d25638e56acdeb8af9e751e703c1a6b45a64e1ca34992c22da946) and baseline identityidentity.json(sha256:799d35042af18a129d9e6e9d9fc4be4e119458d23783cac15b6bf6b314569909).Validation and reproduction
The focused unit coverage lives in
selective_knn_hydration_testsand exercises exact winner application, caller-drop cancellation, typed projection and compatibility fallback, restart and passage preservation, cache admission boundaries, no-row-lock c8 overlap, projection retirement, update/delete and merge races, cache races, timeout integrity, fixed hydration grace, and read-under-write identity boundaries.The mandatory full ES-YAML gate ran against the exact sealed candidate executable after verifying its live
/proc/exehash. It produced 1,360 passed, 0 failed, and 3 skipped with runner exit 0. Local conformancesummary.json(sha256:0a86c22848f8ecc61e3ced5560468dfd1fbf929435027072d8fd6eff81a88a93) andrunner.log(sha256:172c57a49ed0ba1f74bf2208104703ab75676e3ed90a9e4ea9e23cdee25f7dab).The diagnostic was bound by local v7b
artifact-index.json(sha256:6f600fce30b9b907282882e7f882631249b09bf010197691ee03fdddba46309d). The binding fixes lexical embeddings, query cache off, merge interval 3,600 seconds, the pinned 10,000-document/eight-segment index and exhaustive oracle, two repetitions, c1/c8 counterbalancing, one cold wave, eight measured warm waves, and an unmeasured same-clone restart wave. The sealed harness and artifacts are available on request; they are not stored in this PR.The sealed binary predates one post-benchmark, test-only
#[allow(clippy::await_holding_lock)]annotation. Production code is unchanged, but the final commit is therefore not literally the exact source tree used to build the sealed binary.Tradeoffs and limitations
The projection adds resident cache state and approximately 0.968% to the sealed binary. Cache accounting is deliberately conservative and may reject projection publication, in which case the request uses the existing compatibility path. Selected-row decodes can run concurrently under c8 and therefore trade the removed convoy for bounded parallel CPU and read work; the request deadline and blocking-pool limits remain the governors.
The benchmark has two repetitions, external
/procRSS/PSS sampling has jitter, and OS page cache was not controlled. Current main does not expose the prototype semantic route counters or benchmark-authoritative jemalloc telemetry, so this PR does not attribute memory to a specific internal route or allocator category. The benchmark is lexical feature hashing; it does not demonstrate neural embeddings or neural semantic understanding.Scope exclusions and follow-up
This PR does not change filtered, nested, or SQ8 behavior; does not change exact ranking or response semantics for eligible small-index scans; does not claim a FinanceBench result; does not enable the neural embedder; does not alter the ES wire contract; and does not claim to close the broader mixed read-under-write scorecard losses. A longer multi-repetition campaign with controlled OS cache, allocator telemetry, and production route counters would be the appropriate follow-up before turning these diagnostics into a release-level performance claim.