Skip to content

Refactor grid - #776

Open
MarkWolters wants to merge 5 commits into
mainfrom
refactor_grid
Open

MarkWolters wants to merge 5 commits into
mainfrom
refactor_grid

Conversation

@MarkWolters

@MarkWolters MarkWolters commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Refactors Grid, the engine behind BenchYAML and AutoBenchYAML, so concurrent benchmarking work lands in separate files instead of conflicting in Grid.java (now 692 lines, down from 1,395). AutoBenchYAML now shares BenchYAML's pipeline instead of a drifted copy. Build parameters travel as BuildConfig/SearchConfig objects instead of positional arguments, and the hand-nested sweep loops become a GridSpec of SweepDimensions mapped from YAML in GridSpec.fromConfig. Graph construction moves into IndexBuildStrategy implementations backed by a single GraphBuilderFactory, the FeatureId switch becomes one FeatureProvider per on-disk feature, and console tables, experiments.csv rows and AutoBench results become GridListeners that all read build parameters from BuildConfig.reportedParameters(). Command lines, YAML configs, run-config.yml and the experiments.csv schema are unchanged. Intended behavior changes: AutoBench builds one graph per build config instead of rebuilding for every feature set and search compressor, closes its indexes, and adds construction metrics to its JSON; feature sets run in configured order instead of HashMap order; and fused-PQ rows no longer report stale search timings. Also fixes TestBufferedGraphCompatibility, which was flaky on machines without the native vector library because Panama float rounding changes once C2 compiles the code. Harness usage and porting notes are in docs/release notes/4.0.3/776.testing.md.

@github-actions

github-actions Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Before you submit for review:

  • Does your PR follow guidelines from CONTRIBUTIONS.md?
  • Did you summarize what this PR does clearly and concisely?
  • Did you include performance data for changes which may be performance impacting?
  • Did you include useful docs for any user-facing changes or features?
  • Did you include useful javadocs for developer oriented changes, explaining new concepts or key changes?
  • Did you rebase your branch onto the latest main for regression testing and PR submission?
  • Did you trigger regression testing via Run Bench Main and review results?
  • Did you adhere to the code formatting guidelines (TBD)
  • Did you group your changes for easy review, providing meaningful descriptions for each commit?
  • Did you ensure that all files contain the correct copyright header?
  • Did you add documentation for this feature to the release notes directory?

If you did not complete any of these, then please explain below.

@MarkWolters

Copy link
Copy Markdown
Contributor Author
Index_Construction_Time Mean_Latency QPS Recall_at_10

@MarkWolters
MarkWolters marked this pull request as ready for review November 2, 2026 14:53

@tlwillke tlwillke left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for this refactor. This will be much easier to work on and having BenchYAML and AutoBenchYAML share one pipeline is great.

I have a few comments, roughly in order of importance.

1. Wrong build time when the index comes from the cache (Grid.runOneGraph, lines 248–249, 260, 301)
When every feature set for a build config loads from the index cache, built.buildTimeS is null, so indexBuildTimes isn't updated. The values read at lines 260 and 301 then return the previous build config's time for that dataset. This was already true on main, but AutoBench's "Index Build Time" now feeds the regression comparison, so a cached run can report another config's number. Passing built.buildTimeS straight into SearchContext and ConstructionMetrics would fix it and avoid the static map.

2. Indexes not closed when a search fails (Grid.runOneGraph, lines 310–312 and 318)
The indexes are closed only after the search loop finishes normally. If testConfiguration throws, the finally block at line 318 deletes the graph files but leaves the indexes open. AutoBench moves on to the next dataset after a failure, so open files and memory maps pile up, and on Windows the files can't be deleted. Moving the close loop into the finally would fix this.

3. AutoBench result order changed (GridSpec.fromConfig, lines 55–60; Grid.testConfiguration, lines 395 and 397)
AutoBench used to loop M → efConstruction → neighborOverflow → addHierarchy → refineFinalGraph, with topK outside usePruning. It now uses BenchYAML's order: addHierarchy is outermost, and usePruning is outside topK. This is fine if everything that reads AutoBench's JSON matches rows by their parameters. Could you confirm that and mention the change in the release notes alongside the feature-set ordering change?

4. Compressor cache never hits (Grid, lines 361 and 597)
cachedCompressors is an IdentityHashMap, but its keys come from idStringFor(), which builds a new string with String.format on every call. As a result computeIfAbsent never finds an existing entry. Results are still correct because the compressor is reloaded from pq_cache, but every lookup does that reload and the map fills with duplicates. This was already broken on main, but switching to a HashMap is a one-word fix.

5. Possible remaining flakiness (TestBufferedGraphCompatibility.assertSameAnswer, lines 49–60)
Allowing a small tolerance on scores is a good improvement. Node order (line 55) and the visited and reranked counts (lines 58–59) are still compared exactly, though. Those can shift from the same rounding drift when two candidates nearly tie, so a rarer form of the flake may remain. This isn't a blocker; it's only worth revisiting if it shows up in CI again.

Smaller suggestions

6. Sweep setup is duplicated (Bench.main, lines 98–112; Grid.runAllAndCollectResults, lines 513–527)
Both build the same six-dimension GridSpec by hand, in the same order as GridSpec.fromConfig. Adding a construction parameter, which is the main thing this refactor is meant to make easy, means editing three lists and keeping their order in sync. A GridSpec factory that takes the list grids, called from both places, would leave one place to change.

7. Fused-PQ indexes are benchmarked once per search compressor (Grid.runOneGraph, lines 263 and 273)
Indexes with fused PQ ignore the search compressor, but the outer loop over search compressors still runs the full query sweep for each one. With three search compressors, each fused index is benchmarked three times with identical settings, producing rows that differ only in a search-compressor label that wasn't used. Running fused indexes once per build config would save the time and remove the duplicate rows. This was already the case on main.

8. Features without a provider are skipped silently (FeatureProviders.configureWriter, line 82)
If a configured feature has no registered FeatureProvider, the writer prints a message and builds the index without it. The results are still labeled with the requested feature set. Now that adding a provider is the documented way to benchmark a new feature, failing fast when GridSpec is built would catch a missed registration before a long run produces mislabeled results.

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