Add updateable random scorer interface for vector index building #14181
Conversation
lucene/codecs/src/java/org/apache/lucene/codecs/bitvectors/FlatBitVectorsScorer.java
Show resolved
Hide resolved
| @@ -90,23 +91,29 @@ public String toString() { | |||
| private static final class ByteScoringSupplier implements RandomVectorScorerSupplier { | |||
| private final ByteVectorValues vectors; | |||
| private final ByteVectorValues vectors1; | |||
| private final ByteVectorValues vectors2; | |||
| private final VectorSimilarityFunction similarityFunction; | |||
|
|
|||
| private ByteScoringSupplier( | |||
| ByteVectorValues vectors, VectorSimilarityFunction similarityFunction) throws IOException { | |||
| this.vectors = vectors; | |||
| vectors1 = vectors.copy(); | |||
There was a problem hiding this comment.
since it is asymmetric now, instead of 1 vs 2, maybe rename to targetVectors or so?
There was a problem hiding this comment.
I can happily rename everywhere. We use the same pattern many times.
| } | ||
| super.addGraphNode(node, scorer); | ||
| } | ||
|
|
||
| @Override | ||
| public void addGraphNode(int node) throws IOException { |
There was a problem hiding this comment.
curious if we still need this, and generally, if we are still using non-updateable RandomVectorScorer?
There was a problem hiding this comment.
I was unsure about adjusting the full interface :/ There are things that inherit from HNSW graph, and I wanted it to "just work" without any fuss.
| @Override | ||
| public void addGraphNode(int node) throws IOException { | ||
| if (initializedNodes != null && initializedNodes.get(node)) { | ||
| return; | ||
| } | ||
| super.addGraphNode(node); | ||
| if (scorer == null) { | ||
| scorer = scorerSupplier.scorer(node); |
There was a problem hiding this comment.
would it make sense to add this (maybe with node==0?) when creating the builder so it's never null
There was a problem hiding this comment.
Let me see if I can adjust the interface a bit to ensure that we allow a "null" ordinal that simply initializes the buffer.
| for (int node = minOrd; node < maxOrd; node++) { | ||
| addGraphNode(node); | ||
| if (scorer == null) { | ||
| scorer = scorerSupplier.scorer(node); |
There was a problem hiding this comment.
ditto here - these lazy inits are okay, but proactively initializing is better
| to the newly introduced levels (repeating step 2,3 for new levels) and again try to | ||
| promote the node to entry node. | ||
| */ | ||
| UpdateableRandomVectorScorer scorer = scorerSupplier.scorer(node); |
There was a problem hiding this comment.
how did we get away with not doing this before? Is it that we pushed the supplier.scorer() call deeper in the call graph?
There was a problem hiding this comment.
yeah, supplier got pushed all the way down everywhere. Creating new scorers during diversity checking, etc. So, a single node add would likely create many many short lived objects for no useful purpose.
| @@ -463,7 +478,11 @@ private boolean connectComponents(int level) throws IOException { | |||
|
|
|||
| beam.clear(); | |||
| eps[0] = c0.start(); | |||
| RandomVectorScorer scorer = scorerSupplier.scorer(c.start()); | |||
| if (scorer == null) { | |||
There was a problem hiding this comment.
oh yes, here it is -- again could we initialize outside the loop, maybe even initialize to all zeros? Can ScorerSupplier accept null??
As stated by @ChrisHegarty and @msokolov the amount of garbage we create during vector index creation is pretty astounding.
This adjusts the interface to allow an "Updateable" random vector interface (@msokolov 's idea if I remember correctly) and refactors the usage to keep it threadsafe.
I still need to do some larger scale tests, I imagine the actual indexing times are not effected much.
I think I caught all the weird edge cases.