jimczi commented on code in PR #16705:
URL: https://github.com/apache/lucene/pull/16705#discussion_r4093561026
##########
lucene/core/src/java/org/apache/lucene/index/KnnVectorValues.java:
##########
@@ -50,12 +50,47 @@ public int ordToDoc(int ord) {
}
/**
- * Prefetches the provided ordinals
+ * Prefetches {@code count} consecutive vectors starting at the given
ordinal, so that later calls
+ * to read them are more likely to hit memory. Implementations should start
the reads and return
+ * without waiting for them, and are free to prefetch fewer vectors than
asked for, including none
+ * at all. {@code count} is clamped to the number of vectors remaining after
{@code ord}. The
+ * default implementation is a no-op.
+ *
+ * @param ord the ordinal of the first vector to prefetch
+ * @param count how many consecutive vectors to prefetch, starting at {@code
ord}
+ * @return the number of vectors a prefetch was actually issued for, {@code
0} if none, in which
+ * case the caller gains nothing by deferring the reads
+ */
+ public int prefetch(int ord, int count) throws IOException {
+ return 0;
+ }
Review Comment:
prefetch isn't a read, it starts the load and comes back, so a single ord is
useful as long as you read it later. That's exactly what the rerank does, issue
all the candidates first, score after. If you read straight away then yes it
buys nothing, and that case is already handled, the array version skips a lone
ord. `StoredFields#prefetch(int docID)` has the same single item shape.
I also think `(ord, count)` is the right primitive because it's what
`IndexInput#prefetch(offset, length)` takes, and vectors are fixed size and
contiguous by ord. You can build the array version on top of it, which is what
the default does now, but not the other way around. With only the array version
every impl has to find the runs itself, and the one we had didn't, it issued
one madvise per ord. And for a plain range you'd have to allocate an int[] just
to say two numbers.
##########
lucene/core/src/java/org/apache/lucene/index/KnnVectorValues.java:
##########
@@ -50,12 +50,47 @@ public int ordToDoc(int ord) {
}
/**
- * Prefetches the provided ordinals
+ * Prefetches {@code count} consecutive vectors starting at the given
ordinal, so that later calls
+ * to read them are more likely to hit memory. Implementations should start
the reads and return
+ * without waiting for them, and are free to prefetch fewer vectors than
asked for, including none
+ * at all. {@code count} is clamped to the number of vectors remaining after
{@code ord}. The
+ * default implementation is a no-op.
+ *
+ * @param ord the ordinal of the first vector to prefetch
+ * @param count how many consecutive vectors to prefetch, starting at {@code
ord}
+ * @return the number of vectors a prefetch was actually issued for, {@code
0} if none, in which
+ * case the caller gains nothing by deferring the reads
+ */
+ public int prefetch(int ord, int count) throws IOException {
+ return 0;
+ }
+
+ /**
+ * Prefetches the provided ordinals. Ordinals that are consecutive in the
array are prefetched
+ * together with a single call to {@link #prefetch(int, int)}.
*
* @param ordsToPrefetch a list of ordinals to prefetch
* @param numOrds number of ords to prefetch from ordsToPrefetch array
*/
- public void prefetch(final int[] ordsToPrefetch, int numOrds) throws
IOException {}
+ public void prefetch(final int[] ordsToPrefetch, int numOrds) throws
IOException {
+ if (ordsToPrefetch == null) {
+ return;
+ }
+ final int count = Math.min(numOrds, ordsToPrefetch.length);
+ if (count <= 1) {
+ // A single ord is read right after this call, so prefetching it buys no
overlap. Callers that
+ // do want a lone value loaded ahead of time should call prefetch(ord,
1) directly.
+ return;
+ }
+ int i = 0;
+ while (i < count) {
+ final int runStart = i++;
+ while (i < count && ordsToPrefetch[i] == ordsToPrefetch[i - 1] + 1) {
Review Comment:
I don't think `KnnVectorValues` can decide that. Whether 2 and 4 are on the
same page depends on `byteSize`, which only the impl knows. At 1024 dims fp32 a
vector is 4KB so those ords never share a page, at 128 byte dims you get 32 per
page and merging is obviously right.
What I'd rather do is let `MemorySegmentIndexInput#prefetch` remember the
last page aligned range it advised and skip a call that falls inside it. An
IndexInput is single threaded and cloned per thread, so no locking needed. That
covers 2, 4, 6, 8 without any gap heuristic up here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
