-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add support for determining off-heap memory requirements for KnnVectorsReader #14426
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks Chris, this sounds like a nice building block for assessing memory requirements.
...rd-codecs/src/java/org/apache/lucene/backward_codecs/lucene90/Lucene90HnswVectorsReader.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the direct API for gathering "codec off-heap file size". However, I am not sure we are doing that accounting correct in this PR.
I do not like calling it "Off heap requirements". Really, its just the potential size if all the file must be loaded in off-heap.
lucene/core/src/java/org/apache/lucene/util/OffHeapAccountable.java
Outdated
Show resolved
Hide resolved
lucene/core/src/java/org/apache/lucene/util/OffHeapAccountable.java
Outdated
Show resolved
Hide resolved
.../core/src/java/org/apache/lucene/codecs/lucene102/Lucene102BinaryQuantizedVectorsReader.java
Show resolved
Hide resolved
Given the feedback so far, I've pivot this quite a bit to now include per-field metrics. To support this I removed the previously proposed OffHeapAccountable interface and put the accessor on KnnVectorsReader - where other per-field accessors are. I used FieldInfo as the lookup since that it needed to determine if the field is either Byte of Float32, which is required to know in the "richer" codec implementations which provide quantization of floats and passthrough for bytes. |
lucene/core/src/java/org/apache/lucene/codecs/KnnVectorsReader.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM on the new API, I left some nit comments on naming and assertions.
.../core/src/java/org/apache/lucene/codecs/lucene102/Lucene102BinaryQuantizedVectorsReader.java
Show resolved
Hide resolved
lucene/core/src/java/org/apache/lucene/codecs/KnnVectorsReader.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Chris, organization by file extensions is very good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this much better. I do think this requires a CHANGES.txt as it modifies the KnnVectorReader object API. And possibly the API shouldn't be required for all vector readers due to BWC concerns?
lucene/core/src/java/org/apache/lucene/codecs/KnnVectorsReader.java
Outdated
Show resolved
Hide resolved
@ChrisHegarty this PR provides the info on how much off heap space is needed but this doesn't provide info on how much is loaded into memory correct? and do we have any plans to expose the current usage info? |
Hi @navneet1v. You are correct, this is informational only. I do plan to follow up with the actual usage, which may evolve this particular API. |
…rsReader (apache#14426) This PR adds support to KnnVectorsReader in order to determine the off-heap memory requirements. The motivation here is to give better insight into the size of off-heap memory that will be needed, so that deployments can be better scaled so that vector search workloads fit in memory, in order to provide best execution performance.
thanks that will be super useful. |
…nnVectorsReader (#14426) (#14497) This PR adds support to KnnVectorsReader in order to determine the off-heap memory requirements. The motivation here is to give better insight into the size of off-heap memory that will be needed, so that deployments can be better scaled so that vector search workloads fit in memory, in order to provide best execution performance.
…rsReader (apache#14426) This PR adds support to KnnVectorsReader in order to determine the off-heap memory requirements. The motivation here is to give better insight into the size of off-heap memory that will be needed, so that deployments can be better scaled so that vector search workloads fit in memory, in order to provide best execution performance.
This PR adds support to
KnnVectorsReader
in order to determine the off-heap memory requirements.The motivation here is to give better insight into the size of off-heap memory that will be needed, so that deployments can be better scaled so that vector search workloads fit in memory, in order to provide best execution performance.