Swap out some simple PriorityQueue subclasses for one using a Comparator

[thecoop]

Related to #11338 (comment), this replaces some simple uses of PriorityQueue subclasses with ones using a Comparator. This reduces the boilerplate required, the number of classes defined, and moves the definition of the PriorityQueue close to where it is used, so the relevant code is more self-contained and easier to understand.

[thecoop]

This one doesn't need to use a PriorityQueue at all

[uschindler]

Sorting should also be faster here. The Lucene PQ is only needed when items in queue should fall out at botton when its full. In other cases the ln-overhaed is larger than a simple sorting at end.

[github-actions]

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-check label to it and you will stop receiving this reminder on future updates to the PR.

[github-actions]

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-check label to it and you will stop receiving this reminder on future updates to the PR.

[github-actions]

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-check label to it and you will stop receiving this reminder on future updates to the PR.

[thecoop]

Next stage is to run some perf tests to check this doesn't introduce a slowdown due to changes to the virtual method calls in key hotspots.

[uschindler]

Good idea. Why not make PQ final and pass a comparator or maybe a simpler functional interface directly in ctor. This would reduce the number of subclasses more and we only have a single PQ which is final on top.

[uschindler]

This reduces the boilerplate required, the number of classes defined,...

But it does not reduce the number of loaded classes. The lambdas are still hidden classes.

[jpountz]

I like it, it looks more like idiomatic Java. Can you check if OrdinalMapBenchmark reports any slowdown? This is the only benchmark that could be impacted by this change that I can think of.

It's slightly awkward that a PriorityQueue can be defined either using a comparator or overriding lessThan, let's only support providing a comparator as Uwe suggested?

[github-actions]

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-check label to it and you will stop receiving this reminder on future updates to the PR.

[dweiss]

Nice. I feel tempted to make PriorityQueue final and then move the comparator to a final field. Why bother having both ways of defining it?

[thecoop]

This is only the first pass, covering single-line comparators. There are several non-trivial implementations that will require more detailed refactoring (if that is indeed desirable) - using a Comparator requires differentiating equality, which lessThan does not require. There's also some that are part of the public API. I have a subsequent PR covering multi-line comparators, and then several to follow for non-trivial implementations.

[dweiss]

Good point, thanks.

[dweiss]

We could also declare a custom interface with just lessThan and then provide an adapter in usingComparator... this would dodge the need for implementing full equality checks across the board and you could still use jdk's Comparator utilities, where things are simple.

[thecoop]

That's a good idea - I'll look at that when I'm moving onto the more complex implementations

[uschindler]

We should really only have one final PQ implementation to make sure most of the code can be inlined.

[uschindler]

Basically the lessThan interface should be marked @FunctionalInterface and wherever possible the lessTahn check should be a lambda or method ref.

[thecoop]

That's the desired end state, yes. There's still plenty of implementations to get through before we can make it final.

[dweiss]

It would be good to see what the benchmarks show after this change. These pq's are sort of crucial to performance. I wonder if we can observe any change after subclasses are replaced by delegation.

In general, I'm fine to making baby steps although it could also be developed into a larger patch that adds this additional "smaller-than" interface and makes the PQ final. I wonder what others think.

[thecoop]

I wasn't able to run OrdinalMapBenchmark, but a few lucene-bench runs show no large changes - several percent +/- either way

[jpountz]

For reference, there is an interesting PR at #14714 that replaces PriorityQueue with a LongHeap when computing top-k hits by score. Results suggest that the PriorityQueue is not a bottleneck at all when k=100, but is a bottleneck for our fastest queries when k=1000. This PR doesn't change computing top-k hits by field, so tasks TermMonthSort, TermTitleSort, TermDayOfYearSort or TermDTSort would still use a PriorityQueue to track top hits.

[jpountz]

I wasn't able to run OrdinalMapBenchmark

I ran it on my machine. Got the following results on main (2 runs):

id: 651.43124 msec
name: 955.36439 msec
country_code: 0.37753 msec
time_zone: 1.03004 msec

id: 605.82902 msec
name: 923.61305 msec
country_code: 0.30129 msec
time_zone: 0.87078 msec

And the following results on your branch (2 runs as well):

id: 641.80320 msec
name: 945.66611 msec
country_code: 0.31938 msec
time_zone: 0.89374 msec

id: 631.90609 msec
name: 970.12461 msec
country_code: 0.32877 msec
time_zone: 0.91463 msec

So it looks fine to me as far as this benchmark is concerned.

[jpountz]

In general, I'm fine to making baby steps although it could also be developed into a larger patch that adds this additional "smaller-than" interface and makes the PQ final. I wonder what others think.

My intuition was that this "smaller-than" interface idea you brought up would be relatively easy to introduce to all our PriorityQueue impls, so it would be nice if we could skip the intermediate state where there are two ways to define how heap entries are ordered. But maybe I'm underestimating the effort.

[thecoop]

There are some non-trivial subclasses (TopDocs.MergeSortQueue, MultiTermsEnum.TermMergeQueue, NearSpansUnordered.SpanTotalLengthEndPositionWindow), some queues that themselves are subclassed (FieldValueHitQueue, TopOrdAndNumberQueue), and some part of the public API (HitQueue, FieldValueHitQueue, SuggestWordQueue). This PR handles the trivial cases, I was going to create subsequent PRs to cover the more complex refactorings so they can be considered separately, especially those that change the public API.

[dweiss]

I'm fine with merging, given the explanation. Please add a note to changes and migration? Seems like something worth mentioning.

[jpountz]

I'm fine with merging too.

[dweiss]

Merged. Thank you, @thecoop

[uschindler]

Thanks! Cool first step!