Use bulk scoring in NeighborArray#isWorstNonDiverse#15667
Use bulk scoring in NeighborArray#isWorstNonDiverse#15667ML-dev-crypto wants to merge 1 commit intoapache:branch_10xfrom
Conversation
|
@ML-dev-crypto Could you benchmark with lucene util to validate if this is actually worth it? |
| // Allocate a temporary buffer for scores. | ||
| // NeighborArray size is typically small (M=16 or 32), so this allocation is acceptable | ||
| // and keeps the change localized to this class. | ||
| float[] neighborScores = new float[numNodesToCheck]; | ||
|
|
||
| // Bulk score all neighbors. | ||
| // The default implementation in RandomVectorScorer handles the fallback if needed. | ||
| scorer.bulkScore(nodes.buffer, neighborScores, numNodesToCheck); |
There was a problem hiding this comment.
please, delete all the useless LLM-esque comments. We can see what its doing. Comments describing "what" are fairly useless.
There was a problem hiding this comment.
Understood. I've removed the verbose comments and kept the documentation focused on the 'why' rather than the 'what'.
| } | ||
|
|
||
| // Bulk score all unchecked neighbors | ||
| scorer.bulkScore(nodesToCheck, neighborScores, numNodesToCheck); |
There was a problem hiding this comment.
Why can't use use the max score returned from bulk score?
There was a problem hiding this comment.
THANKS FOR YOUR GUIDANCE I've updated the logic to capture the return value from bulkScore and use it for an early exit. If the maximum similarity in the batch is worse than the candidate, we can return immediately without iterating.
| float minAcceptedSimilarity = scores.get(candidateIndex); | ||
| if (candidateIndex == uncheckedIndexes[uncheckedCursor]) { | ||
| // the candidate itself is unchecked | ||
| for (int i = candidateIndex - 1; i >= 0; i--) { |
There was a problem hiding this comment.
Could we have the scratch data passed in here?
There was a problem hiding this comment.
You should be able to pass in int[] and float[]. I am not sure why you only passed in one.
I suggest maybe using the DocAndFloatFeatureBuffer
There was a problem hiding this comment.
THANKS FOR YOUR GUIDANCE AND TIME.Right now this PR removes the dominant allocation by reusing the float[] in the hot path. There is still a small int[] allocation when gathering unchecked nodes, which could be avoided by passing both int[] and float[] or by using DocAndFloatFeatureBuffer.
I kept this PR minimal and behavior-preserving, but I’m happy to extend it to reuse both buffers if you’d prefer that in this change.
Reuse a scratch buffer during HNSW diversity checks and use RandomVectorScorer.bulkScore with early termination to avoid per-call allocations on the hot path.
ML-dev-crypto
force-pushed
the
branch_10x
branch
from
eaecdc4 to
f9073c4
Compare
|
@ML-dev-crypto do we know what is the impact in performance because of this change? |
Thanks for the question. Theoretically, this change eliminates array allocations in the hot path of graph construction and utilizes vector API bulk scoring, which should reduce GC pressure and improve throughput. |
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution! |
3 participants
This change updates NeighborArray#isWorstNonDiverse to use the
RandomVectorScorer#bulkScore API, avoiding repeated per-neighbor
score calls in a hot loop.
The bulkScore method is a default interface method and safely falls
back to per-node scoring when not overridden by a scorer.
Fixes #15606