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

BREAKING: Introducing ValuePriorityQueue<T> #826

Merged
merged 6 commits into from
Apr 14, 2023

Conversation

NightOwl888
Copy link
Contributor

@NightOwl888 NightOwl888 commented Apr 12, 2023

This adds a new ref struct to the project, Lucene.Net.Util.ValuePriorityQueue<T>. This struct contains the most of the same API and business logic as PriorityQueue<T>, but allows the memory used to be allocated elsewhere as Span<T> and passed in.

The amount to allocate can be calculated by calling PriorityQueue.GetArrayHeapSize(maxSize), where maxSize is the external size of the PriorityQueue (representing the same value that is passed into the constructor of the current PriorityQueue<T>). The actual allocation amount is 1 greater than the priority queue size because it is 1-based rather than 0-based.

We get around the issue with structs not being inheritable by introducing a new PriorityComparer<T> abstract class with a single bool LessThan(T a, T b) method. This signature makes converting existing subclasses of PriorityQueue<T> to PriorityComparer<T> straightforward, as there are no business logic changes to make. This IComparer<T> is a required constructor argument of ValuePriorityQueue<T>. It can be either a PriorityComparer<T> or IComparer<T>, although if it returns a 0 (equal), that value is considered the same as 1 (greater than) by the priority queue.

This also modifies SingleTaxonomyFacets, Int32TaxonomyFacets, TopDocs merge and FST pack to use ValuePriorityQueue<T> and converts the item types they deal with from class to struct so they can be stack allocated if the size is below Constants.MaxStackByteLimit, which is set using the system property maxStackByteLimit.

Closes #774. Thanks @eladmarg for the suggestion.

There are a few more places in the codebase where this type can be useful, but unfortunately the types they use don't convert easily into structs so although we can eliminate the heap allocation of the priority queue itself, we will need some configuration and/or logic to decide whether to allocate the array directly or use an array pool.

Breaking Changes

  1. Changed OrdAndValue<T> from class to struct.
  2. Added generic constraint to OrdAndValue<T> where T : struct.

…ate the per-request PriorityQueue<T> allocation in certain instances.
… TopOrdAndSingleQueue + TopOrdAndInt32Queue): Converted OrdAndValue<T> into a struct. Modified to stack allocate both the priority queue instance and the buffer that backs it, the latter falling back to using an array pool if the number of bytes to allocate exceeds Constants.MaxStackByteLimit. Closes apache#774.
… and allocate the buffer on the stack if it fits within Constants.MaxStackByteLimit.
…locate the buffer on the stack if it fits within Constants.MaxStackByteLimit.
@NightOwl888 NightOwl888 requested a review from laimis April 12, 2023 23:20
Copy link
Contributor

@laimis laimis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a great change! Nicely done!

@eladmarg
Copy link
Contributor

fantastic!
sometimes the priority queue size is so small, no need even to rent and allocate on the stack is much faster.

by the way, net 7 finally introduce PriorityQueue implementation, however this one seems to be more specific and faster.

…accept IComparer<T> instead of IPriorityComparer<T>. This will save allocations of adapter classes for IComparer<T>. Replaced IPriorityComparer<T> with an abstract class PriorityComparer<T> that implements IComparer<T> and makes a crude conversion that ignores equal to (0) and assumes either greater than (1) or less than (-1) when LessThan() returns false or true respectively.
@NightOwl888 NightOwl888 merged commit 4b8ef79 into apache:master Apr 14, 2023
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.

3 participants