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

LUCENE-10137: Refactor DirectReader and remove heavy code duplication #335

Closed
wants to merge 1 commit into from

Conversation

uschindler
Copy link
Contributor

@uschindler uschindler self-assigned this Sep 30, 2021
@jpountz
Copy link
Contributor

jpountz commented Sep 30, 2021

I'm curious what luceneutil will report with this change, the sorting and faceting tasks should give us a good idea of whether there is a slowdown or not.

@uschindler
Copy link
Contributor Author

Actually I will do a perf test in a minute, but the code and call behaviour here is more or less identical to before. Instead of creating the classes in source code we let the JDK do it for us.

The only additional "overhead" is the get() method which calls the functional interface, but thats always inlined. It could be prevented when (as noted in the issue) we change LongValues to be a functional interface on its own. Currently it is an abstract class with one method, which is a desaster,

@uschindler
Copy link
Contributor Author

Looks like there's a slowdown with facets. Digging (waiting for CPU profiles)...

@rmuir
Copy link
Member

rmuir commented Sep 30, 2021

where is the "heavy code duplication" ? You still have method to decode for each bit size. But now we made things more complicated for hotspot.

@uschindler
Copy link
Contributor Author

uschindler commented Sep 30, 2021

Hi,
I will close this PR because with JDK 11 it is problematic.

I improved the whole thing a bit by changing LongValues to be an interface instead of an abstract class, which made the DirectReader look like this:

https://github.com/uschindler/lucene/blob/18e52b2b64bdc4020a32581935e0d44774aed39a/lucene/core/src/java/org/apache/lucene/util/packed/DirectReader.java

As you see it is mush shorter and better readable, but for some reason it does not behave well yet. I will keep the other branch in my own repository for later investigation.

The whole experiment is here: main...uschindler:jira/LUCENE-10137-v2

@uschindler uschindler closed this Sep 30, 2021
@uschindler
Copy link
Contributor Author

FYI, with JDK 18 the other branch works fast, it is just Java 11 that cannot handle this.

@uschindler uschindler deleted the jira/LUCENE-10137 branch September 30, 2021 12:05
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.

None yet

3 participants