Skip to content

Reject duplicate neighbor IDs while preserving score order - #738

Draft
tlwillke wants to merge 1 commit into
mainfrom
fix-neighbor-id-uniqueness
Draft

tlwillke wants to merge 1 commit into
mainfrom
fix-neighbor-id-uniqueness

Conversation

@tlwillke

@tlwillke tlwillke commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to #522. This takes a narrower approach on current main, preserving score-sorted neighbors and the existing diverse-prefix optimization rather than changing storage to ID order.

  • Reject duplicate neighbor IDs regardless of score, retaining the established score.
  • Deduplicate candidate merges, including initial batches, while preserving equal-score interleaving.
  • Reject duplicate offers before copying, growing, or evicting entries, without triggering pruning.
  • Use direct-array membership checks and reuse the checked insertion position during cleanup.

No scorer, query algorithm, or disk-format changes. Existing graphs are not repaired on load; normal candidate merges may deduplicate individual neighborhoods. Different IDs with identical vectors remain distinct.

Recap

Duplicate node IDs show up in graphs built using asymmetric scoring because A dot quant(B) /= B dot quant(A) and the current code only rejects nodes with BOTH the same node ID and score. This damages search due to redundant search paths at all levels in the graph. This does not affect graph indexes built with full precision.

Validation

27 focused tests passed on JDK23 with the jdk20 Maven profile: TestNodeArray, TestNeighbors, GraphIndexBuilderTest. Coverage includes different-score duplicates, full arrays, merge tails, tie ordering, concurrent offers, degrees 2–2048, duplicate rejection without pruning, and full-precision graph construction. Replacement is tested to evict the lowest score rather than the highest ID.

Performance — pending

Draft pending a BenchYAML comparison of frozen main e31aa59 versus this fix: FP and PQ on Ada002-1M, CAP-1M, Cohere-1M; repeated fresh graphs in balanced order with shared PQ compressors; construction time, single-thread QPS, visited count, recall@10 at 1x/2x, and duplicate audits at every level. Both branches will use identical benchmark harness code. Material regressions will be investigated and reported before requesting readiness.

@github-actions

github-actions Bot commented Sep 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.

@tlwillke tlwillke added the bug Something isn't working label Sep 30, 2026
@tlwillke

tlwillke commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator Author

PQ benchmark results — performance not cleared

Completed 12 fresh PQ builds (two per arm per dataset), 12 retained-index query retests, and four separate CAP JFR construction profiles. Baseline e31aa595; candidate a4be0e9b. The earlier fallback-reader runs are excluded.

Configuration: JDK 23, Panama 512-bit SIMD, verified MemorySegmentReader, 48 construction workers, single-thread queries; degree 64, efConstruction 200, overflow 1.2, hierarchy/refinement enabled, search pruning disabled; PQ mFactor=8, k=256, uncentered, fused PQ + NVQ reranking. Shared codebooks and identical benchmark overlays. Build order main/fix/fix/main.

Means below; QPS is from the separate retained-index retest with sequential warming, verified initial residency, three real-query warmup passes, and three samples of at least 10 seconds. Initial residency does not guarantee residency throughout execution.

Dataset Arm Build seconds Warm QPS 1x / 2x Recall@10 % 1x / 2x Visited 1x / 2x
Ada002-1M main 582.73 2673.7 / 2369.3 61.000 / 80.951 773.3 / 1081.9
Ada002-1M fix 544.20 2442.5 / 2434.2 61.215 / 81.140 808.2 / 1115.5
CAP-1M main 278.99 2302.8 / 2188.2 56.851 / 77.099 722.4 / 1005.8
CAP-1M fix 766.13 2545.0 / 2414.8 56.936 / 77.264 769.3 / 1053.8
Cohere-English-v3-1M main 180.22 3370.5 / 3160.3 66.831 / 85.852 823.3 / 1200.4
Cohere-English-v3-1M fix 728.80 3152.3 / 2884.2 66.810 / 85.758 899.0 / 1276.5

Correctness

Every fixed graph has zero duplicate neighbor IDs at every level. Baseline extra duplicate entries per build: Ada002 180,869 / 180,227; CAP 199,123 / 199,720; Cohere 770,411 / 769,181. The 27 focused regression tests passed. Recall differences are small, but two builds do not establish statistical significance.

Timing limitations and profiling

Do not interpret construction means as an isolated deduplication cost. Raw build times (seconds): Ada main 249.24/916.22, fix 201.76/886.65; CAP main 316.40/241.58, fix 645.70/886.56; Cohere main 187.54/172.90, fix 784.38/673.22. All outliers are retained. Query timing also varies across JVMs despite warming.

Separate CAP JFR runs locate most slow-run time in insertion. Insertion times main/fix/fix/main were 1337.00/1277.67/227.96/869.51 seconds (diagnostic timings, not pooled above). Slow runs on both branches heavily sample PanamaVectorUtilSupport.assembleAndSumPQ_512 line 949 → IntVector.intoArray → VectorSupport.store → Java lane-by-lane stOp fallback loops. The fast fixed-branch run lacks material sampling in those fallback loops. Selecting Panama does not guarantee every operation is intrinsified. This identifies a shared hot path; the precise compiler/intrinsic cause and isolated deduplication overhead remain unresolved.

Recommendation: keep this PR draft. Correctness is demonstrated; performance is not cleared. Investigate the shared PQ compilation instability separately before requesting readiness. No ASH, reader implementation, query scorer, or format changes are included in this PR.

@tlwillke

tlwillke commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator Author

CAP-100k construction diagnosis: shared PQ/JIT instability

Follow-up on the construction-time variability above. Tested exactly the first 100,000 CAP vectors, with the same shared PQ codebook, graph parameters, JDK23, 48 construction workers, Panama512, and verified MemorySegmentReader. No recall measurement for this prefix. Production artifacts remain unchanged.

Uninstrumented insertion times in main/fix/fix/main order: 15.84 / 55.53 / 21.09 / 83.05 seconds. Both branches exhibit the problem.

Four separate JFR/compiler-log runs gave insertion 57.82 / 21.48 / 19.71 / 21.86 seconds. All reached C2 early. The slow kernel lacks _VectorStoreOp; all three fast kernels have it. Slow stacks include assembleAndSumPQ_512 → IntVector.intoArray → Java store fallback.

Separate timing instrumentation measured 1.251B vs 1.258B insertion diversity comparisons (+0.59%), but sampled cost rose from 670 ns to 2,869 ns per comparison. Search scoring did not slow down. Wrappers can change JIT decisions; these are diagnostic measurements, not isolated branch-overhead estimates.

Allocation is a major part of this: GC logs show approximately 5.21 TB cumulatively reclaimed in the slow-main profiled JVM versus 12.9 GB in fast main (rounded before/after heap sizes, whole runs, not resident memory). JFR allocation stacks identify temporary vector objects and backing arrays in PQ conversions/gather/add operations. Short GC pauses alone substantially understate the allocation cost.

Controls, again main/fix/fix/main insertion seconds:

  • Suppress only the kernel store intrinsic: 80.99 / 38.77 / 83.64 / 78.98. This generates an inlined fallback and is not identical to the naturally slow path.
  • Disable tiered compilation: 133.40 / 164.79 / 166.08 / 142.64. This also loses load/gather intrinsics; C2 rejects the two fromByteSequence calls and fromVectorFloat because the helpers are already compiled into big methods.

During the observed 164.79-second insertion, system I/O pressure was only 0.007 seconds over the 162.36-second monitored window, system iowait 0.006%, and no Java D-state threads were sampled. This argues against disk stalls as the main cause of that run. Separate writeInline contention and one 24.59-second flush outlier remain documented; they are not being discarded.

Keep this PR draft. The shared PQ compilation/allocation problem is established, but the exact trigger for the default-mode store-intrinsic failure and a production remedy remain unresolved. Stabilize that scorer before interpreting the large 1M build differences as deduplication overhead. Neither diagnostic JVM flag is a recommended production setting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant