Repository navigation
Repros 747 - #752
Closed
jshook wants to merge 51 commits into
Closed
Repros 747#752jshook wants to merge 51 commits into
jshook wants to merge 51 commits into
Conversation
forBenchmark(MultiConfig) now returns an explicitly set jvector.dataset_loader as-is, so buffered applies even to uncompressed-construction and exact-search configurations, matching the documented explicit override. Routing still applies when buffered input comes from the YAML useDatasetPrefetching flag. Fixes TestDatasetLoaderOverride.
randomAccessScoreProvider's value-shared path re-read node1 on every comparison, doubling source reads in full-precision builds from a file-backed source. The anchor is now copied on first use and kept by the score function: creation still reads nothing, the copy can't be overwritten by later reads through the same views, and each comparison reads only node2. Fixes TestBuildScoreProviderAnchorReads.
Storage.close() cleared each point reader's channel and buffer without taking its lease, so a concurrent point read could hit a null buffer or reopen a channel that was never closed. close() now takes each reader's lease before closing its channel, and readPointInto checks for closure after taking a lease, so later point reads fail with IllegalStateException. Fixes TestFvecCloseDuringPointReads.
…n one thread Grid now supplies NVQ/FP features at the final write for every run, but only the parallel writer calls those suppliers from several threads. BenchYAML's default (sequential) writer calls them on one thread, so all final-write vector reads and NVQ encoding run single-threaded after construction. On Cohere-1M build+write went from 74-76 s to 100-104 s, and on OpenAI-3072-1M from 165 s to 264 s. Fails at ec42a94 (PR #747).
Grid moved NVQ/FP feature encoding into the final write for every run, but the sequential writer (BenchYAML's default) calls final suppliers on one thread. Grid now defers features only when every writer is an OnDiskParallelGraphIndexWriter; otherwise it encodes them on the insertion threads and writes them with writeFeaturesInline, as before #747. The docs and release note say so. Fixes TestGridDeferredFeatureParallelism.
awaitTasks joined every task but never stopped them, so an interrupted or failed write serialized every remaining record before reporting. writeL0Records now shares a stop flag with its tasks: awaitTasks sets it on interruption or the first task failure, unstarted tasks return at once, and running tasks stop before their next record. Every task is still joined before the channel and suppliers are released. NodeRecordTask keeps its existing constructor. Fixes TestParallelWriterInterrupt.
The test built each graph on a 3-thread ForkJoinPool, so insertion order, hierarchy-level draws and refinement cleanup varied with thread timing. Its self-match check failed intermittently under full-suite load for the BQ, 768-dimension, DOT_PRODUCT, refined configuration (node 5 missing from its own top 10). Graphs are now built through builder.build(source) with single-worker executors, and the file-backed graph's adjacency must equal a resident-built reference graph at every level. That checks the test's purpose (a shared file source must not overwrite insertion or diversity queries) edge for edge. The self-match check is kept and is now deterministic for the fixed data.
jshook
requested review from
MarkWolters,
ashkrisk and
tlwillke
as code owners
October 13, 2026 01:28
Contributor
|
Before you submit for review:
If you did not complete any of these, then please explain below. |
Collaborator
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
2 participants
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.
Review findings: repros and fixes
These were reviewed at
ec42a94b. The repro tests and proposed fixes are onrepros-747, one commit each:16772ea7c7470e3d935d02fa68afc073NodeRecordTask(hygiene only)TestBufferedGraphCompatibility(from #744) fails intermittently under load7b4679131. Default writer regression. Grid now always defers NVQ/FP encoding to the final write.
parallelBuilddefaults tofalseand BenchYAML has no way to change it. So plain BenchYAML and CI runs use the sequential writer, which does all of that encoding on one thread after cleanup. On Cohere-1M, build+write went from 74–76 s to 100–104 s, and on OpenAI-3072-1M from 166 s to 264 s. The PR's measurements all enable the parallel writer, so they don't show this. The fix defers only when the parallel writer is in use. Otherwise Grid encodes during insertion and callswriteFeaturesInline, as before. With the fix, Cohere-1M takes 72.8 s. The docs and release note are updated to match.2. Interrupt or failure doesn't stop the write.
awaitTaskscorrectly joins every task before the channel and suppliers are released, but nothing tells the tasks to stop. An interrupted or failed write still serializes every remaining record before it reports. The repro interrupts after 50 records: it stops 3.2 s later, after supplying 6000 of 6000 records. The fix adds a stop flag thatawaitTaskssets on an interrupt or the first failure. Tasks check it before each record, and every task is still joined.Future.cancelis not used, because it would break the join.3. Dead padding.
payloadBytesalways equalsrecordSize, sopaddingis always 0 and the padding writes never run. Remove the padding code, or replace it with an assertion.4. Flaky #744 test. The test builds the graph on a 3-thread FJP, so the topology depends on thread timing. Under load, one BQ/DOT_PRODUCT/refine case occasionally misses its self-match. The fix builds every graph with
ForkJoinPool(1). It also builds a reference graph from resident vectors and asserts that the adjacency is identical at every level. That checks directly that buffered vectors don't get clobbered, which is the test's purpose. The change touches only the test file, so it can be cherry-picked onto #744.With the three fixes, the full native suite (
-Djvector.vectorization_provider=native) passes: 429 tests, 0 failures, 2 skipped.