Remove vector values copy() methods, moving IndexInput.clone() and temp storage into lower-level interfaces #13872

msokolov · 2024-10-07T21:28:47Z

addresses #13831

The basic idea is to move the scratch arrays and cloned IndexInputs (generally, any stateful data) into things returned by KnnVectorValues, so that class itself no longer needs to be cloned in order to get unique sources of vectors (or scorers). ByteVectorValues and FloatVectorValues got a new vectors() method (this returns the "dictionary") that supports the random access. Also, RandomVectorScorer gets cloned inputs and scratch data when created rather than relying on getting these from its enclosing values instance.

Naming notes:

The issue calls for a "dictionary" interface, but I found the name a bit confusing, so I undertook the following renaming: The "dictionary" interface is represented by FloatVectorValues.Floats and ByteVectorValues.Bytes (hearkening back to the RandomAccessVectorValues classes) and these new objects are returned by the new *VectorValues.vectors() methods. Where these methods are called, I've changed the names of the variables storing these things to vectors. Instance of KnnVectorValues are now mostly stored in variables called vectorValues; these were called various things before including vectors, values, or vectorValues. I left some called values since I didn't want to touch any more more files.

I've renamed the method vectorValue(int ord) to get(int ord) since there were entirely too many vectors, values, and vectorValues running around.

I also ensured that KnnVectorValues.iterator() always returns a unique instance. Previously we had been caching in a few places and returning a shared instance, which seems like a bug, although I don't think it caused any problems given our usage.

All in all it's a lot of fussy non-functional changes but I do think the clarity makes it worth doing now after ~5 years of evolution of these APIs

…of 0)

msokolov · 2024-10-08T14:25:02Z

I'll merge to main soon and let tests noodle on this for a few days before backporting to 11.x. It seems benign, but it's easy to make an accidental slip in the code hurricane

benwtrent

The API does look cleaner, but I am concerned about heap and performance during graph building.

addAndEnsureDiversity will create many copies. I would expect this PR to create many more float[dim] arrays than we would before.

Have you done any benchmarking or profiling on this?

benwtrent · 2024-10-08T17:46:06Z

...ward-codecs/src/java/org/apache/lucene/backward_codecs/lucene94/OffHeapByteVectorValues.java

+    return new Bytes() {
+      IndexInput input = slice.clone();
+      ByteBuffer byteBuffer = ByteBuffer.allocate(byteSize);
+      ;


Suggested change

;

benwtrent · 2024-10-08T18:07:46Z

...ne/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99ScalarQuantizedVectorsReader.java

+      Floats rawVectors = rawVectorValues.vectors();
+      return new Floats() {
+        @Override
+        public float[] get(int ord) throws IOException {
+          return rawVectors.get(ord);
+        }
+      };


Suggested change

Floats rawVectors = rawVectorValues.vectors();

return new Floats() {

@Override

public float[] get(int ord) throws IOException {

return rawVectors.get(ord);

}

};

return rawVectorValues.vectors();

benwtrent · 2024-10-08T18:13:10Z

lucene/core/src/java/org/apache/lucene/codecs/lucene99/OffHeapQuantizedByteVectorValues.java

+      ByteBuffer byteBuffer = ByteBuffer.allocate(dimension);
+      byte[] binaryValue = byteBuffer.array();
+      IndexInput input = slice.clone();
+      float[] scoreCorrectionConstant = new float[1];


all these should be private & final. There are other instances where you do something similar, let's make things final that can be and private things can should be.

personally I don't care about making these final - the compiler already ensures that they are or it wouldn't let you use them in a closure like this. As for private, I don't think you can make local variables private, but maybe I am missing something.

lucene/core/src/java/org/apache/lucene/index/ExitableDirectoryReader.java

lucene/core/src/java/org/apache/lucene/index/SortingCodecReader.java

benwtrent · 2024-10-08T18:22:44Z

.../org/apache/lucene/internal/vectorization/Lucene99MemorySegmentByteVectorScorerSupplier.java

+      byte[] scratch1 = new byte[vectorByteSize];
+      byte[] scratch2 = new byte[vectorByteSize];


now we allocate scratch even if we don't need it, maybe this isn't that big of a deal?

Same goes for all the other memsegment scorers, we don't really need the scratch unless a memory segment isn't available.

Yeah, this just seemed cleaner than trying to make that conditional, and my assumption is these scorers are not created that often? Once per search? Although I guess when indexing that could be a lot (once per doc). The challenge here is that getSegment() is a member of the Supplier while the Scorers are the ones that should be supplying the scratch data, so we can't easily create scratch lazily. I guess we could create some new abstraction in here to handle that but it seems kind of messy.

Is there some way to know "up front" whether a memorysegment is going to be produced? If we knew that we could allocate scratch space or not based on that knowledge. I have to say I'm a little lost in this java21 MemorySegment code -- maybe @ChrisHegarty will weigh in and explain what the conditions are that lead to segmentSliceOrNull returning null?

We don't know during construction whether or not access to the vector data in backing segment will always be available. The main reason is that a vector may span across multiple memory segments. (one MSIndexInput can be made up of several memory segments)

This change is not right. The scratch buffers were created per supplier, since we know from the threading model that that is safe. Creating scratch buffers per scorer will be too expensive.

I have another idea. maybe we just delegate the null cases to the other on-heap scorer. That might be simpler. We do something similar in the native scorer we have in Elasticsearch. I can see how this looks in the branch, if u like?

I'm not sure I understand your idea, Chris, but if you want to have a go at it, by all means please do, and maybe I'll understand then :)

benwtrent · 2024-10-08T18:32:27Z

lucene/core/src/java/org/apache/lucene/codecs/hnsw/DefaultFlatVectorScorer.java

+    public RandomVectorScorer scorer(int ord) throws IOException {
+      ByteVectorValues.Bytes vectors1 = vectorValues.vectors();
+      ByteVectorValues.Bytes vectors2 = vectorValues.vectors();
+      return new RandomVectorScorer.AbstractRandomVectorScorer(vectorValues) {


I would expect this to create way more garbage during HNSW graph building. The RandomVectorScorerSupplier is passed around to the diverse checking, which will now, on each scorer that is created (which will likely be many of them for every node we add), we allocate new scratch space. Before, we had a single set of scratch space created just in the RandomVectorScorerSupplier.

I worry this will have a measurable performance impact and hurt heap usage.

yeah this seems like a bad consequence. Maybe we could switch from a supplier/scorer to a mutable scorer that can be "set" to a new vector as needed?

msokolov · 2024-10-08T21:00:46Z

Thanks for the insightful feedback - yeah I had been intending to do perf testing, and then got distracted by fascinating talks and kind of forgot about these concerns! Going through the code adding all these allocations I was kind of thinking most of them would be infrequent, but I agree if we are creating scorers per node that isn't going to be acceptable, so we need to find a way of sharing just enough but not too much. Anyway there's no rush to get this in, I'll take some time to dig in.

msokolov · 2024-10-09T03:43:17Z

hm there is some functional problem with the change that yields terrible recall for quantized vectors. I'll dig and fix and see if I can beef up the unit test coverage as well.

benwtrent · 2024-10-09T11:32:19Z

hm there is some functional problem with the change that yields terrible recall for quantized vectors. I'll dig and fix and see if I can beef up the unit test coverage as well.

This likely means somewhere the scratch space isn't being appropriately handled :/

…in Lucene90HnswVectorsReader

jpountz

I find it much cleaner this way too.

Unrelated, we should add support for absolute reads of float[] arrays and implement these dictionaries on top of a RandomAccessInput instead of an IndexInput. (for a follow-up PR)

jpountz · 2024-10-16T07:18:59Z

lucene/MIGRATE.md

@@ -892,3 +892,7 @@ segments are rewritten either via `IndexWriter.forceMerge` or
 ### Vector values APIs switched to primarily random-access

 `{Byte/Float}VectorValues` no longer inherit from `DocIdSetIterator`. Rather they extend a common class, `KnnVectorValues`, that provides a random access API (previously provided by `RandomAccessVectorValues`, now removed), and an `iterator()` method for retrieving `DocIndexIterator`: an iterator which is a DISI that also provides an `index()` method. Therefore, any iteration over vector values must now be performed using the values' `iterator()`. Random access works as before, but does not require casting to `RandomAccessVectorValues`.
+
+## Migration from Lucene 10.0 to Lucene 10.1


Should it be at the top? This file has most recent versions at the top.

msokolov · 2024-10-22T19:07:57Z

With the most recent commit I saw these luceneutil/knnPerfTest.py results:

1. baseline

recall  latency (ms)     nDoc  topK  fanout  maxConn  beamWidth  quantized  index s  force merge s  num segments  index size (MB)
 0.816         0.294  1500000    10       6       32         50         no   341.37         110.92             1          1534.03
 0.811         0.308  1500000    10       6       32         50     7 bits   346.68          93.22             1          1906.16
 0.786         0.288  1500000    10       6       32         50     4 bits   346.28          89.15             1          1906.10

this change with defaults (no command line flags)

recall  latency (ms)     nDoc  topK  fanout  maxConn  beamWidth  quantized  index s  force merge s  num segments  index size (MB)
 0.817         0.304  1500000    10       6       32         50         no     344.11      111.70             1          1533.94
 0.812         0.231  1500000    10       6       32         50     7 bits     354.29       89.76             1          1906.16
 0.785         0.239  1500000    10       6       32         50     4 bits     352.37        89.01             1          1906.12

This change with vector api enabled:

recall  latency (ms)     nDoc  topK  fanout  maxConn  beamWidth  quantized  index s  force merge s  num segments  index size (MB)
 0.817         0.247  1500000    10       6       32         50         no     0.00           0.17             1          1533.94
 0.812         0.282  1500000    10       6       32         50     7 bits     0.00           0.17             1          1906.16
 0.785         0.207  1500000    10       6       32         50     4 bits     0.00           0.17             1          1906.12

This change with vector api and enable-native-access

recall  latency (ms)     nDoc  topK  fanout  maxConn  beamWidth  quantized  index s  force merge s  num segments  index size (MB)
 0.817         0.246  1500000    10       6       32         50         no     0.00           0.17             1          1533.94
 0.812         0.290  1500000    10       6       32         50     7 bits     0.00           0.17             1          1906.16
 0.785         0.206  1500000    10       6       32         50     4 bits     0.00           0.18             1          1906.12

So I think there is some slowdown in the quantized indexing. I think we need to find a solution for the over-allocations due to having moved this logic from ScorerSupplier to Scorer. The best idea I have is to make Scorers mutable and supply them with new target vectors as needed. WDYT?

jpountz · 2024-10-25T13:10:19Z

Can you clarify which allocation is the problematic one, and where it's done on the indexing path?

benwtrent · 2024-10-25T13:43:32Z

I think a "merging scorer" would be good. The only place the "scorer supplier" is used is during graph building.

My initial concern with a "mutable scorer" is that it would also make the single scorer mutable, which seems weird to me. But I am happily to revisit this, especially since its blocking a nice refactor.

Given that all these random scorer stuff is internal APIs, we can do whatever is best with what we have.

msokolov · 2024-10-25T13:50:37Z

Yes, OK I now see quite a bit of this is a "preexisting condition" and maybe not exacerbated by this change. We are still creating more scratch arrays than we did before though, I think, because previously we would copy() the VectorValues in a caller, and allocate a new scratch array there, whereas now since we have pushed down the "create new scratch array" into the Scorer creation, and this happens many more times than we would previously have copied the VectorValues, we are creating and destroying many more of these scratch arrays. Maybe this is acceptable and we can iterate in a futher cleanup? Let me try a few more benchmarking runs and be a little clearer about the impact on query and indexing times. I'd like to also report allocations, but not sure how to do that w/luceneutil

msokolov · 2024-10-25T13:57:36Z

Maybe we could add a RandomVectorScorer.setTarget(int node) method that would only be implemented by the Scorers returned from ScorerSuppliers?

ChrisHegarty · 2024-10-29T17:03:44Z

Maybe we could add a RandomVectorScorer.setTarget(int node) method that would only be implemented by the Scorers returned from ScorerSuppliers?

Let's defer a double addressing scorer to follow on change, since this already getting big!

I pushed a change to fix the off-heap scorer. The scratch buffers have been deferred to only when needed, and only per null on the slice, which is rare.

ChrisHegarty · 2024-10-30T12:02:51Z

FYI - I created the following issue to track the possibility of adding a scorer interface that scores one ordinal against another, without the need to create an instance of a scorer. #13966

msokolov · 2024-11-06T15:39:34Z

I re-tested this change, including @ChrisHegarty 's latest, and it still exhibits some bug that causes terrible recall for quantized vectors. I think it is only occurring when concurrent merging is enabled, so I'll dig around in there. Separately this seems to point to more gaps in our unit test coverage. I'll open a separate issue to enhance RandomCodec so it can explore different KnnVectorsFormat settings (like concurrent merges).

msokolov · 2024-11-06T19:25:06Z

In the process of adding a randomized KnnVectorsFormat to RandomCodec I learned that 4-bit quantization is not supported for odd-length vectors !? This makes the randomized testing quite difficult since we don't have a good way of ensuring that the 4-bit codec generated randomly by trhe test framework never comes in contact with the odd-dimensional vectors randomly generated by the test case. I wonder how hard it would be to actually support this?

benwtrent · 2024-11-06T19:41:49Z

I wonder how hard it would be to actually support this?

It requires some logic and changes to optimized scoring. The purpose for preventing it was:

It makes the logic for int4 compression & scoring way simpler
Odd dimensioned vectors are rare or not used at all in practice

msokolov · 2024-11-06T20:59:57Z

Odd dimensioned vectors are rare or not used at all in practice

true - maybe we ought to ban outright? Otherwise this is kind of weird. One thing we could do is pad on the input side. I believe all our similarities would be invariant to adding an extra zero?

msokolov · 2024-11-07T12:47:07Z

Finally got back to this and fixed the aliasing that was happening. I ran some perf tests and don't see significant variance. Still, it's clear we must be doing a lot more allocations here during indexing; I plan to look at profiler output to see that. But maybe it's captured by escape analysis and we're able to re-use? I was thinking a possible solution to that could be to maintain a pool of Scorers in each ScorerSupplier and reclaim them with a close()? But maybe that can come as a follow up.

this patch

recall  latency (ms)     nDoc  topK  fanout  maxConn  beamWidth  quantized  index s  force merge s  num segments  index size (MB)
 0.965         2.995  1000000    10      64       64        250         no  1795.97        1329.54             1          3018.62
 0.878         2.844  1000000    10      64       64        250     7 bits  1715.70        1202.92             1          3749.72
 0.541         2.084  1000000    10      64       64        250     4 bits  1650.77         842.20             1          3764.59

mainline, Cohere, mip, 1M docs

recall  latency (ms)     nDoc  topK  fanout  maxConn  beamWidth  quantized  index s  force merge s  num segments  index size (MB)
 0.965         3.161  1000000    10      64       64        250         no  1813.15        1277.61             1          3018.62
 0.878         2.775  1000000    10      64       64        250     7 bits  1681.21        1168.95             1          3749.72
 0.541         2.092  1000000    10      64       64        250     4 bits  1646.78         802.31             1          3764.59

msokolov · 2024-11-07T12:49:45Z

hmm that commit is kind of messed up. Maybe I missed a rebase on main somewhere? I will try to clean up here, but it might require a force-push or a new PR, egad

msokolov · 2024-11-07T15:51:59Z

heap comparison

Here's the output from luceneutil's JFR heap usage summarizer. Clearly a huge amount more allocations for this change.

float32 mainline

PERCENT       HEAP SAMPLES  STACK
31.03%        262M          org.apache.lucene.util.hnsw.NeighborArray#<init>() [Inlined code]
18.07%        152M          org.apache.lucene.util.ArrayUtil#copyOfSubArray() [Inlined code]
9.11%         77M           java.lang.Integer#valueOf() [Inlined code]
8.61%         72M           org.apache.lucene.util.hnsw.NeighborQueue#nodes() [Inlined code]

float32 knn-dictionary

PERCENT       HEAP SAMPLES  STACK
94.25%        54257M        org.apache.lucene.codecs.lucene95.OffHeapFloatVectorValues#vectors() [Inlined code]
2.27%         1306M         org.apache.lucene.store.MemorySegmentIndexInput#clone() [Inlined code]
1.14%         656M          org.apache.lucene.util.hnsw.NeighborQueue#nodes() [Inlined code]
0.46%         266M          org.apache.lucene.store.MemorySegmentIndexInput$SingleSegmentImpl#<init>() [Inlined code]
0.43%         245M          org.apache.lucene.util.hnsw.NeighborArray#<init>() [Inlined code]
0.37%         214M          org.apache.lucene.codecs.hnsw.DefaultFlatVectorScorer$FloatScoringSupplier#scorer() [JIT compiled code]
0.24%         138M          org.apache.lucene.util.ArrayUtil#copyOfSubArray() [Inlined code]
0.14%         83M           java.lang.Integer#valueOf() [Inlined code]
0.11%         62M           org.apache.lucene.codecs.lucene95.OffHeapFloatVectorValues#vectors() [JIT compiled code]
0.07%         42M           org.apache.lucene.util.hnsw.NeighborQueue#nodes() [JIT compiled code]

int7 mainline

PERCENT       HEAP SAMPLES  STACK
53.56%        14617M        jdk.internal.foreign.SegmentFactories#fromArray() [Inlined code]
41.52%        11333M        jdk.internal.foreign.MemorySessionImpl#createHeap() [Inlined code]
1.16%         316M          org.apache.lucene.codecs.lucene99.Lucene99ScalarQuantizedVectorScorer#dotProductFactory() [Inlined code]
1.03%         280M          org.apache.lucene.util.hnsw.NeighborArray#<init>() [Inlined code]
0.56%         152M          org.apache.lucene.util.ArrayUtil#copyOfSubArray() [Inlined code]
0.29%         79M           java.lang.Integer#valueOf() [Inlined code]

int7 knn-dictionary

PERCENT       HEAP SAMPLES  STACK
40.22%        20231M        java.nio.HeapByteBuffer#<init>() [Inlined code]
30.42%        15298M        jdk.internal.foreign.SegmentFactories#fromArray() [Inlined code]
22.30%        11214M        jdk.internal.foreign.MemorySessionImpl#createHeap() [Inlined code]
1.35%         677M          org.apache.lucene.store.MemorySegmentIndexInput#clone() [Inlined code]
1.01%         506M          java.nio.ByteBuffer#allocate() [Inlined code]
0.82%         411M          org.apache.lucene.codecs.lucene99.OffHeapQuantizedByteVectorValues$1#<init>() [Inlined code]
0.77%         389M          org.apache.lucene.codecs.lucene99.Lucene99ScalarQuantizedVectorScorer#dotProductFactory() [Inlined code]
0.63%         315M          org.apache.lucene.codecs.lucene99.OffHeapQuantizedByteVectorValues#vectors() [Inlined code]
0.54%         270M          org.apache.lucene.util.hnsw.NeighborArray#<init>() [Inlined code]
0.31%         155M          org.apache.lucene.store.MemorySegmentIndexInput$SingleSegmentImpl#<init>() [Inlined code]
0.31%         154M          org.apache.lucene.util.ArrayUtil#copyOfSubArray() [Inlined code]
0.15%         75M           java.lang.Integer#valueOf() [Inlined code]

int4 mainline

PERCENT       HEAP SAMPLES  STACK
26.75%        348M          org.apache.lucene.codecs.lucene99.Lucene99ScalarQuantizedVectorScorer#dotProductFactory() [Inlined code]
21.15%        275M          org.apache.lucene.util.hnsw.NeighborArray#<init>() [Inlined code]
11.16%        145M          org.apache.lucene.util.ArrayUtil#copyOfSubArray() [Inlined code]
6.36%         82M           java.lang.Integer#valueOf() [Inlined code]
5.25%         68M           org.apache.lucene.util.hnsw.NeighborQueue#nodes() [Inlined code]
2.79%         36M           org.apache.lucene.util.SparseFixedBitSet#insertLong() [JIT compiled code]
2.54%         33M           java.util.Arrays#copyOf() [Inlined code]

int4 knn-dictionary

85.00%        20633M        java.nio.HeapByteBuffer#<init>() [Inlined code]
2.53%         614M          org.apache.lucene.store.MemorySegmentIndexInput#clone() [Inlined code]
2.05%         498M          java.nio.ByteBuffer#allocate() [Inlined code]
1.66%         403M          org.apache.lucene.codecs.lucene99.OffHeapQuantizedByteVectorValues$1#<init>() [Inlined code]
1.60%         388M          org.apache.lucene.codecs.lucene99.Lucene99ScalarQuantizedVectorScorer#dotProductFactory() [Inlined code]
1.16%         282M          org.apache.lucene.codecs.lucene99.OffHeapQuantizedByteVectorValues#vectors() [Inlined code]
1.14%         277M          org.apache.lucene.util.hnsw.NeighborQueue#nodes() [Inlined code]
1.05%         255M          org.apache.lucene.util.hnsw.NeighborArray#<init>() [Inlined code]
0.81%         196M          org.apache.lucene.store.MemorySegmentIndexInput$SingleSegmentImpl#<init>() [Inlined code]
0.60%         144M          org.apache.lucene.util.ArrayUtil#copyOfSubArray() [Inlined code]
0.34%         82M           java.lang.Integer#valueOf() [Inlined code]
0.18%         44M           org.apache.lucene.util.hnsw.NeighborQueue#nodes() [JIT compiled code]
0.16%         38M           org.apache.lucene.util.SparseFixedBitSet#insertLong() [JIT compiled code]
0.15%         36M           java.util.Arrays#copyOf() [Inlined code]

ChrisHegarty · 2024-11-07T16:15:03Z

thanks @msokolov I'll take another look at why the off-heap scorer is allocating so much.

msokolov · 2024-11-07T19:46:00Z

@ChrisHegarty I think it's expected since we would previously do the allocation once per KnnVectorValues, but now we are doing it once per RandomVectorScorer. I'm working on adding Closeable to some of these interfaces so that we can pool and reuse these objects. Maybe there's another way?

ChrisHegarty · 2024-11-08T10:39:01Z

The threading and reuse model in the current API is subtle, but works well, as it mostly minimises the potential for garbage.

Looking at this again, it's almost like anywhere there was a copy in scorers, we need to replace it with vectors in the constructor. Rather than creating a new instance each and every time on usage. Lemme see how this looks.

ChrisHegarty · 2024-11-08T11:52:24Z

@msokolov ok, this is that I was thinking - f1e0007. There might be other places too, this was just a start to see how it fairs in benchmarks.

ChrisHegarty · 2024-11-08T12:08:15Z

Looking again in a bit more detail, I dislike that it is not possible to get access to the vector values without always creating at least one slice. Previously this was not necessary. The separation of concerns that copy offered from access was subtle but worked. I'd like to better understand how much this will affect things. Maybe my last change was sufficient, and the extra slice is washed away in the noise!? I'll try to do some benchmarking on this too.

ChrisHegarty · 2024-11-08T12:27:06Z

I am now completely confused by this change. And I think that my last commit broke the threading model - even though all tests pass!. I'm not saying that we should not do some refactoring here, but I think that the original premise that this change is based upon is not quite right.

We have the following use cases:

single-threaded: where the scoring can be performed by many scorer instances backed by two slices. Since the scorers can only possibly be scoring sequentially, and we can benefit from sharing the effective thread-local buffer.
multi-threaded: we want to fork a new score supplier so that it can be used in a separate thread. Here, obviously, we want a new slice that is unaffected by the creating thread, and which will have it's own effective thread-local buffer.

My understanding is that conflating access and cloning/copying into a single method means that we cannot achieve both the above use cases in an optimal way.

msokolov · 2024-11-08T13:17:53Z

OK; re allocating vectors in supplier, yes that can't work because of aliasing across threads. I also found we do not adequately test this case. There are many things in here that are not tested; I think we need to add randomization in RandomCodec as we have for other codecs (postings, docvalues) to get better coverage, and we should also explicitly be testing many of the codec options we have added (multithreaded merges, different forms of quantization). Maybe we have coverage for some of these things, but it looks to me as if our format-specific tests just choose default options.

My understanding is that conflating access and cloning/copying into a single method means that we cannot achieve both the above use cases in an optimal way.

I am coming to a similar conclusion. Although I do think this API change can be made to work if we cache the scorers in the suppliers and return them when done (I was thinking of doing this by making them closeable), this does add a lot of extra bookkeeping and I'm not sure it's going to end up being better in the end.

As a side note, I see that IndexInput is Closeable -- but I don't think we ever close() the ones we allocate? Is this a problem?

ChrisHegarty · 2024-11-08T15:23:27Z

As a side note, I see that IndexInput is Closeable -- but I don't think we ever close() the ones we allocate? Is this a problem?

Not a problem per se. The creator of the scorers need to ensure that the primary IndexInput is closed. Clones do not need to be closed. My understanding is that all these primary index inputs are closed.

(I was thinking of doing this by making them closeable), this does add a lot of extra bookkeeping and I'm not sure it's going to end up being better in the end.

I'm not against this, but I remain to be convinced that it is worth it.

There are many things in here that are not tested;

++ its separate, but I completely agree that we need better tests.

msokolov · 2024-11-08T17:06:49Z

the last commit demonstrates re-using the resource-intensive bits (RandomVectorScorer and Floats) for the floarting point case. It brings back heap allocations to be similar to what we see on main.

msokolov

I recommend turning off whitespace diffs when viewing this; a bunch of it is indentation because of adding try-with-resources blocks

msokolov · 2024-11-08T17:08:58Z

lucene/core/src/java/org/apache/lucene/index/FloatVectorValues.java

@@ -118,6 +123,9 @@ public Floats vectors() {
          public float[] get(int ord) throws IOException {
            return vectors.get(ord);
          }
+
+          @Override


oops this is redundant

msokolov · 2024-11-08T17:09:32Z

...ne/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99ScalarQuantizedVectorsReader.java

+
+        @Override
+        public void close() throws IOException {
+          rawVectors.close();


these changes to the quantized reader/writer might be needed, but not just for the Floats case

ChrisHegarty · 2024-11-08T17:39:48Z

the last commit demonstrates re-using the resource-intensive bits (RandomVectorScorer and Floats) for the floarting point case. It brings back heap allocations to be similar to what we see on main.

Ok, this is great - we correctly understand where the allocation was coming from and have a way to avoid it.

I'm still not sure that this refactor is better than the model that we have today. And the need to make the scorer closable kinda hints towards that. ( I'm not against the fact that the scorer is closable, just that it adds little value to the API and is not enforced - we don't want to enforce it's closed status )

msokolov · 2024-11-08T18:02:02Z

Yeah I think I will stop working on this. Where I've ended up is with suspicions about the ScorerSupplier/Scorer setup. Because we allocate Scorers a lot we rely on that being a cheap operation, but ideally we should not have to create an object (actually a factory for creating objects, and then the object) in order to compare two vectors. The VectorValues knows where its vectors are, maybe it ought to do the comparison and worry about whether it needs to copy or can avoid it. Although when we repetitively compare against a known vector, that is one a case where it makes sense to me that we'd want to create an object representing that cached value-comparison.

Michael Sokolov added 13 commits October 7, 2024 09:22

refactor float vector values random access

0b76ac3

refactor byte vector values random access

1c2977f

make sure KnnVectorValues.iterator() always returns a new value

debac32

fix cloning/sharing of vector scorer resources

273e8ed

renaming

ce70f4c

tidy

b4febca

more renaming

2fca27c

EMPTY

2e51380

CHANGES and MIGRATE entries

3b8d70f

a little more renaming

f5e0260

mopping up some more values->vectors

9c68a6e

fix javadoc

f035183

fix error introduced in refactoring (init lastSubIndex to -1 instead …

23c7497

…of 0)

benwtrent reviewed Oct 8, 2024

View reviewed changes

Michael Sokolov added 2 commits October 14, 2024 11:17

Add BaseKnnVectorsFormatTestCase.testRecall() and fix map ord to doc …

63a4d83

…in Lucene90HnswVectorsReader

Add BaseKnnVectorsFormatTestCase.testRecall() and fix map ord to doc …

2099589

…in Lucene90HnswVectorsReader

jpountz reviewed Oct 16, 2024

View reviewed changes

Michael Sokolov added 7 commits October 17, 2024 05:12

handle stray prints

6141900

test all similarities and more queries

61a0d79

fix Lucene90Hnsw that was aliasing vector values

5a6d709

remove stray print

bbe4d28

Merge remote-tracking branch 'origin/main' into knn-dictionary

1a2c3bb

fix initialization bug in SlowCompositeCodecReaderWrapper

568372f

simplifications from PR feedback

da06288

fix off-heap scorer by falling back to on-heap.

ed233ba

Merge branch 'main' into knn-dictionary

5c2cb2d

fix aliasing of vector scratch in quantized scorer

4284360

msokolov force-pushed the knn-dictionary branch from c35bc10 to 4284360 Compare November 7, 2024 12:52

update flat vectors scorer to use only two vector dictionaries

f1e0007

reuse Floats and RandomVectorScorers

ef13bad

msokolov commented Nov 8, 2024

View reviewed changes

		byte[] scratch1 = new byte[vectorByteSize];
		byte[] scratch2 = new byte[vectorByteSize];

Remove vector values copy() methods, moving IndexInput.clone() and temp storage into lower-level interfaces #13872

Are you sure you want to change the base?

Remove vector values copy() methods, moving IndexInput.clone() and temp storage into lower-level interfaces #13872

Conversation

msokolov commented Oct 7, 2024 • edited Loading

msokolov commented Oct 8, 2024

benwtrent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisHegarty Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msokolov commented Oct 8, 2024

msokolov commented Oct 9, 2024

benwtrent commented Oct 9, 2024

jpountz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msokolov commented Oct 22, 2024

1. baseline

this change with defaults (no command line flags)

This change with vector api enabled:

This change with vector api and enable-native-access

jpountz commented Oct 25, 2024

benwtrent commented Oct 25, 2024

msokolov commented Oct 25, 2024

msokolov commented Oct 25, 2024

ChrisHegarty commented Oct 29, 2024

ChrisHegarty commented Oct 30, 2024 • edited Loading

msokolov commented Nov 6, 2024

msokolov commented Nov 6, 2024

benwtrent commented Nov 6, 2024

msokolov commented Nov 6, 2024 • edited Loading

msokolov commented Nov 7, 2024

this patch

mainline, Cohere, mip, 1M docs

msokolov commented Nov 7, 2024

msokolov commented Nov 7, 2024

heap comparison

float32 mainline

float32 knn-dictionary

int7 mainline

int7 knn-dictionary

int4 mainline

int4 knn-dictionary

ChrisHegarty commented Nov 7, 2024

msokolov commented Nov 7, 2024

ChrisHegarty commented Nov 8, 2024

ChrisHegarty commented Nov 8, 2024

ChrisHegarty commented Nov 8, 2024

ChrisHegarty commented Nov 8, 2024

msokolov commented Nov 8, 2024

ChrisHegarty commented Nov 8, 2024

msokolov commented Nov 8, 2024

msokolov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisHegarty commented Nov 8, 2024

msokolov commented Nov 8, 2024

msokolov commented Oct 7, 2024 •

edited

Loading

ChrisHegarty Oct 25, 2024 •

edited

Loading

ChrisHegarty commented Oct 30, 2024 •

edited

Loading

msokolov commented Nov 6, 2024 •

edited

Loading