Skip to content
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

Implement IntersectVisitor#visit(IntsRef) whenever it makes sense #14138

Merged
merged 15 commits into from
Jan 16, 2025

Conversation

iverase
Copy link
Contributor

@iverase iverase commented Jan 14, 2025

In #13149, the method Implement IntersectVisitor#visit(IntsRef) was introduced which is a nice. trick to avoid multiple virtual calls to IntersectVisitor#visit(int).

This was only implemented for PointRangeQuery but it is useful for all implementation that expects hitting that code path. Therefore in this PR we proposed to implement this method wherever it make sense.

@jpountz
Copy link
Contributor

jpountz commented Jan 15, 2025

This makes sense to me, if the visit() call is inlined, then feeding the IntsRef into either an int[] (BufferAdder) or a FixedBitSet (BitSetAdder) can be auto-vectorized. Otherwise it can't.

This makes me wonder if we should instead introduce BulkAdder#add(IntsRef) and implement these visit(IntsRef) methods by calling adder.add(intsRef) directly, this would make auto-vectorizing less likely to get disabled by an apparently innocent change.

@iverase
Copy link
Contributor Author

iverase commented Jan 15, 2025

Thanks @jpountz. I added the method to the BulkAdder. The good thing is that for the buffer case we can use System.arraycopy which is even better.

@iverase
Copy link
Contributor Author

iverase commented Jan 15, 2025

Unrelated test error:

./gradlew test --tests TestFeatureField.testStoreTermVectors -Dtests.seed=BEC58524A0EDCFC6 -Dtests.locale=kw-Latn-GB -Dtests.timezone=Pacific/Kosrae -Dtests.asserts=true -Dtests.file.encoding=UTF-8

@iverase
Copy link
Contributor Author

iverase commented Jan 15, 2025

I made BulkAdder sealed so we are sure they will always just two implementations.

@iverase
Copy link
Contributor Author

iverase commented Jan 15, 2025

Actually we can make BulkAdder a sealed interface and the implementations are Java records.

@jpountz
Copy link
Contributor

jpountz commented Jan 15, 2025

Unrelated test error

@jimczi This test is failing because we go through a slightly different code path on the first doc of a segment, which generates a slightly different exception message it seems.

@iverase iverase added this to the 10.2.0 milestone Jan 16, 2025
@iverase iverase merged commit e4b85ca into apache:main Jan 16, 2025
5 checks passed
@iverase iverase deleted the intersectVisitor_vc branch January 16, 2025 06:52
iverase added a commit that referenced this pull request Jan 16, 2025
…4138)

Implement IntersectVisitor#visit(IntsRef) in many of the current implementations and add
BulkAdder#add(IntsRef) method. They should provide better performance due to less virtual 
method calls and more efficient bulk processing.
@jpountz
Copy link
Contributor

jpountz commented Jan 16, 2025

I opened a PR for the test failure: #14146

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

Successfully merging this pull request may close these issues.

2 participants