PostingsDecodingUtil: interchange loops to enable better memory access and SIMD vectorisation #15110
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
|
@dweiss, Kindly, can you please merge as well, Thank you! |
rmuir
approved these changes
Aug 22, 2025
|
Should we do the same in |
|
Thanks @jpountz, for taking a look at this PR. Yes, MemorySegmentPostingDecodingUtil has the same strided-write pattern. I’ll open a follow-up PR that flips the loops so the innermost store becomes the contiguous |
|
If this is to be backported then the changes entry should go into 10x. |
|
Got it @dweiss. moved the change entry from 11.0 to 10.3 section. |
|
. |
|
I ran main: this PR: I only ran with main: this PR: |
|
Yeah... I'm no longer so convinced we should accept micro-benchmarking results without looking at overall performance. When you run the code in a different context it seems to compile differently. Weird. |
…s and SIMD vectorisation
RamakrishnaChilaka
force-pushed
the
postings_decoding_util_improvements
branch
from
fe12ff6 to
90835c5
Compare
|
In this PR, we targeted optimising the scalar path by rearranging loops so that memory accesses are sequential rather than strided, which gave a measurable performance improvement (see comment, the JMH only tests for scalar path). I applied the same optimization to the vectorized path, but it didn’t yield the expected improvement — decodeVector performance actually regressed compared to baseline (as also verified by Adrian). I didn't run microbenchmarks for it prior to this. I suspect this is because, in the mainline vectorized version, we are loading from the memory segment only for that lane. To achieve the same contiguous write pattern as the scalar version, we would need to load the entire vector from memory at once, and this may be causing the regression. I have reproduced this behavior consistently. Please check decodeVector and decodeAndPrefixSumVector below. Summary of the perf changes:
I also ran the benchmarks across all DPVs with this PR (only PostingDecodingUtil changes, MemorySegmentDecodingUtil reverted). As expected, the scalar version shows consistent improvements, while the vectorized version remains mostly unchanged (as expected, as we didn't change its flow). Summarising the perf results:
TLDR;Key takeaway:
|
uschindler
approved these changes
Aug 26, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PostingsDecodingUtil: interchange loops to enable better memory access and SIMD vectorisation.
Description
Rewrite splitInts() so the innermost loop walks the array contiguously instead of strided manner. Giving chance to HotSpot C2 to vectorise the shift-and-mask operations and also improves cache locality.
No functional change - the temporary buffer
bis only accessed insidethis method and its layout order is irrelevant to callers.
Performance comparsion on intel i5-13600k
Full JMH results are in the benchmark repo.
Speed ups:
Summarising: