Skip to content

Conversation

yossev
Copy link

@yossev yossev commented Aug 1, 2025

Replaced the two-step prefix sum loop in Lucene99HnswVectorsReader with a single-loop variant that avoids redundant memory access and improves performance.

Previous approach:

  • Read first value separately.
  • Then used previous buffer element + readVInt().

New approach:

  • Accumulates sum in a single pass and assigns directly.

This change follows the suggestion from issue #14979 and has the same functional behavior with slightly better efficiency.

Replaced the two-step prefix sum loop in `Lucene99HnswVectorsReader` with a single-loop variant that avoids redundant memory access and improves performance.

Previous approach:
- Read first value separately.
- Then used previous buffer element + readVInt().

New approach:
- Accumulates sum in a single pass and assigns directly.

This change follows the suggestion from issue apache#14979 and has the same functional behavior with slightly better efficiency.
Copy link
Contributor

github-actions bot commented Aug 1, 2025

This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR.

@jpountz
Copy link
Contributor

jpountz commented Aug 1, 2025

Thank you, this looks good. If you have cycles to run benchmarks, this would be appreciated, you can check out this: https://github.com/mikemccand/luceneutil/blob/main/README.md#running-the-knn-benchmark.

@yossev
Copy link
Author

yossev commented Aug 1, 2025

Thank you, this looks good. If you have cycles to run benchmarks, this would be appreciated, you can check out this: https://github.com/mikemccand/luceneutil/blob/main/README.md#running-the-knn-benchmark.

Thank you, Glad it looks good.
Before proceeding, I just wanted to check, is there anything you'd like me to adjust or improve before it's ready to be merged?

Also, I’ll take a look at the benchmark tool you linked and try to run some comparisons if time allows. Appreciate the reference!

@jpountz
Copy link
Contributor

jpountz commented Aug 1, 2025

The only missing thing is an entry in lucene/CHANGES.txt but we can deal with it later.

@yossev
Copy link
Author

yossev commented Aug 1, 2025

Perfect.
I will be waiting for updates on that matter.
Thank you!

@jpountz
Copy link
Contributor

jpountz commented Aug 8, 2025

@yossev FYI you'll need to resolve conflicts since another change touched the same lines of code. I'm curious if you had any success with benchmarking?

@yossev
Copy link
Author

yossev commented Aug 9, 2025

@yossev FYI you'll need to resolve conflicts since another change touched the same lines of code. I'm curious if you had any success with benchmarking?

Thanks for the heads-up! I noticed that the recent changes in Lucene99HnswVectorsReader.java reverted part of the optimization I made according to #14979
I’ll resolve the merge conflicts by keeping that fix (unless there’s a reason it should be reverted), and will adjust it if needed to fit the new changes.

Regarding benchmarking ,I’ll run it after the conflicts are resolved so results reflect the latest code.

@msokolov
Copy link
Contributor

msokolov commented Aug 9, 2025

I was checking to see what had been reverted, but I don't see it? #14979 didn't change Lucene99HnswVectorReader.java, right? Maybe you're referring to the conflicts since #14932 moved the VInt-decoding block into a conditional?

@yossev
Copy link
Author

yossev commented Aug 9, 2025

I was checking to see what had been reverted, but I don't see it? #14979 didn't change Lucene99HnswVectorReader.java, right? Maybe you're referring to the conflicts since #14932 moved the VInt-decoding block into a conditional?

I was just giving credit to #14979 to the optimization i made in this case, not saying it changed anything.

Copy link
Contributor

This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR.

@yossev
Copy link
Author

yossev commented Aug 13, 2025

Hello @jpountz,

Please check the latest commit, i resolved the conflicts locally, and added back the optimization that was mentioned #14979.

i will be waiting for further feedback.

@jpountz
Copy link
Contributor

jpountz commented Aug 13, 2025

@yossev see the output of the build failure, the build is unhappy with formatting, you need to run ./gradlew tidy. Have you had any success with benchmarking?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants