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

[API Proposal]: Add a DequeueEnqueue method to the PriorityQueue Type #75070

Closed
Tracked by #77391
RyanThomas73 opened this issue Sep 4, 2022 · 17 comments · Fixed by #75993
Closed
Tracked by #77391

[API Proposal]: Add a DequeueEnqueue method to the PriorityQueue Type #75070

RyanThomas73 opened this issue Sep 4, 2022 · 17 comments · Fixed by #75993
Labels
api-approved API was approved in API review, it can be implemented area-System.Collections
Milestone

Comments

@RyanThomas73
Copy link
Contributor

RyanThomas73 commented Sep 4, 2022

Background and motivation

When working with d-ary heap structures, insert-then-extract and extract-then-insert operations can be done which are generally more efficient than sequentially executing the separate insert/extract operations.

The existing PriorityQueue<TElement, TPriority> collection type has an existing method signature named EnqueueDequeue which is an optimized implementation of the insert-then-extract operation. It does not have an api method for the inverse extract-then-insert operation.

The term pop-push is sometimes used for the extract-then-insert implementation. The python implementation calls this heapreplace: https://docs.python.org/3/library/heapq.html#heapq.heapreplace

The optimized extract-then-insert operation is ideal for use cases where each dequeue operation is likely or guaranteed to be followed by an enqueue operation.

One example usage of the new method would be a scenario where the elements of the priority queue represent nodes from different linked lists. If each operation to dequeue the next element is always followed by inserting the next node that is pointed at, the optimized extract-then-insert would be the preferable choice.

API Proposal

namespace System.Collections.Generic;

public class PriorityQueue<TElement, TPriority>
{
        /// <summary>
        ///  Removes the minimal element and then immediately adds the specified element with associated priority to the <see cref="PriorityQueue{TElement, TPriority}"/>,
        /// </summary>
        /// <param name="element">The element to add to the <see cref="PriorityQueue{TElement, TPriority}"/>.</param>
        /// <param name="priority">The priority with which to associate the new element.</param>
        /// <returns>The minimal element removed before performing the enqueue operation.</returns>
        /// <remarks>
        ///  Implements an extract-then-insert heap operation that is generally more efficient
        ///  than sequencing Dequeue and Enqueue operations
        /// </remarks>
        public TElement DequeueEnqueue(TElement element, TPriority priority)
        {
            if (_size == 0)
                throw new InvalidOperationException(SR.InvalidOperation_EmptyQueue);

            (TElement Element, TPriority Priority) root = _nodes[0];

            if (_comparer == null)
            {
                if (Comparer<TPriority>.Default.Compare(priority, root.Priority) > 0)
                {
                    MoveDownDefaultComparer((element, priority), 0);
                }
                else
                {
                    _nodes[0] = (element, priority);
                }
            }
            else
            {
                if (_comparer.Compare(priority, root.Priority) > 0)
                {
                    MoveDownCustomComparer((element, priority), 0);
                }
                else
                {
                    _nodes[0] = (element, priority);
                }
            }

            _version++;
            return root.Element;
        }
}

API Usage

public record ListNode(ListNode? Next, int Value);

var priorityQueue = new PriorityQueue<ListNode, int>(initialElements);
while (priorityQueue.TryPeek(out var peekedElement, out _))
{
    var minElement = peekedElement.Next != null
        ? priorityQueue.DequeueEnqueue(peekedElement.Next, peekedElement.Next.Value)
        : priorityQueue.Dequeue();
        
    // ... logic that uses minElement ....
}

Alternative Designs

N/A

Risks

The new method implementation must be correctly optimized in order to achieve better performance than executing the Dequeue() and Enqueue(..) methods sequentially.

@RyanThomas73 RyanThomas73 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Sep 4, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Sep 4, 2022
@ghost
Copy link

ghost commented Sep 4, 2022

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

When working with d-ary heap structures, insert-then-extract and extract-then-insert operations can be done which are generally more efficient than sequentially executing the separate insert/extract operations.

The existing PriorityQueue<TElement, TPriority> collection type has an existing method signature named EnqueueDequeue which is an optimized implementation of the insert-then-extract operation. It does not have an api method for the inverse extract-then-insert operation.

The term pop-push is sometimes used for the extract-then-insert implementation. The python implementation calls this heapreplace: https://docs.python.org/3/library/heapq.html#heapq.heapreplace

One example usage of the new method would be a scenario where the elements of the priority queue represent nodes from different linked lists. If each operation to dequeue the next element is always followed by inserting the next node that is pointed at, the optimized extract-then-insert would be the preferable choice.

API Proposal

namespace System.Collections.Generic;

public class PriorityQueue<TElement, TPriority> : IEnumerable<T>
{
        /// <summary>
        ///  Removes the minimal element and then immediately adds the specified element with associated priority to the <see cref="PriorityQueue{TElement, TPriority}"/>,
        /// </summary>
        /// <param name="element">The element to add to the <see cref="PriorityQueue{TElement, TPriority}"/>.</param>
        /// <param name="priority">The priority with which to associate the new element.</param>
        /// <returns>The minimal element removed before performing the enqueue operation.</returns>
        /// <remarks>
        ///  Implements an extract-then-insert heap operation that is generally more efficient
        ///  than sequencing Dequeue and Enqueue operations
        /// </remarks>
        public TElement DequeueEnqueue(TElement element, TPriority priority)
        {
            if (_size == 0)
                throw new InvalidOperationException(SR.InvalidOperation_EmptyQueue);

            (TElement Element, TPriority Priority) root = _nodes[0];

            if (_comparer == null)
            {
                if (Comparer<TPriority>.Default.Compare(priority, root.Priority) > 0)
                {
                    MoveDownDefaultComparer((element, priority), 0);
                }
                else
                {
                    _nodes[0] = (element, priority);
                }
            }
            else
            {
                if (_comparer.Compare(priority, root.Priority) > 0)
                {
                    MoveDownCustomComparer((element, priority), 0);
                }
                else
                {
                    _nodes[0] = (element, priority);
                }
            }

            _version++;
            return root.Element;
        }
}

API Usage

public record ListNode(ListNode? Next, int Value);

var priorityQueue = new PriorityQueue<ListNode, int>(initialElements);
while (priorityQueue.TryPeek(out var peekedElement, out _))
{
    var minElement = peekedElement.Next != null
        ? priorityQueue.DequeueEnqueue(peekedElement.Next, peekedElement.Next.Value)
        : priorityQueue.Dequeue();
        
    // ... logic that uses minElement ....
}

Alternative Designs

N/A

Risks

The new method implementation must be correctly optimized in order to achieve better performance than executing the Dequeue() and Enqueue(..) methods sequentially.

Author: RyanThomas73
Assignees: -
Labels:

api-suggestion, area-System.Collections

Milestone: -

@krwq krwq added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Sep 5, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Sep 5, 2022
@krwq krwq added this to the Future milestone Sep 5, 2022
@krwq
Copy link
Member

krwq commented Sep 5, 2022

Proposal looks good to me. Note we're not sure when this will get picked up by API review. Assuming it gets approved are you interested in creating PR with implementation and tests?

@krwq
Copy link
Member

krwq commented Sep 5, 2022

Schedule is here: https://apireview.net/ - if you offer help I can bump milestone to 8.0 to increase priority

@teo-tsirpanis
Copy link
Contributor

Should we also have a TryDequeueEnqueue variant?

@RyanThomas73
Copy link
Contributor Author

Should we also have a TryDequeueEnqueue variant?

It's an option. Seems like it could be ambiguous on what the behavior would be though; e.g. would it still enqueue if there was nothing to dequeue first? Also, unlike the DequeueEnqueue signature, I wouldn't expect a significant performance difference with checking using a sequential call to TryPeek first.

Proposal looks good to me. Note we're not sure when this will get picked up by API review. Assuming it gets approved are you interested in creating PR with implementation and tests?

Yes I can put together an implementation and tests PR.

@krwq krwq modified the milestones: Future, 8.0.0 Sep 6, 2022
@krwq
Copy link
Member

krwq commented Sep 6, 2022

Ok, you're now somewhere in the middle of the list up for review (I'm guessing it will take at most couple of weeks), you can update the proposal with TryDequeEnqueue and similar, make sure to be consistent with remainder of the APIs

@vslee
Copy link

vslee commented Sep 7, 2022

Seems like it could be ambiguous on what the behavior would be though; e.g. would it still enqueue if there was nothing to dequeue first?

Perhaps TryDequeEnqueue() could take an additional bool parameter which specifies whether to enqueue if there was nothing to dequeue first?

@krwq
Copy link
Member

krwq commented Sep 7, 2022

another option is to simply throw, if you're calling that API rather than just enqueue you most likely have bug in your app

@eiriktsarpalis
Copy link
Member

@RyanThomas73 for completeness, could you point out a couple of use cases/algorithms where using extract-then-insert is beneficial?

@eiriktsarpalis
Copy link
Member

Should we also have a TryDequeueEnqueue variant?

This method seems redundant. A queue.Count > 0 check should suffice to verify that the operation will succeed.

@RyanThomas73
Copy link
Contributor Author

RyanThomas73 commented Sep 7, 2022

@RyanThomas73 for completeness, could you point out a couple of use cases/algorithms where using extract-then-insert is beneficial?

Hi Eric. Expanding on my background information/example, one common use case is when the heap/priority queue needs to be kept within a fixed initial size/max size and the additional elements added in as the next min is dequeued.

public record ListNode(ListNode? Next, int Value);
ListNode producerOne = ... get producer data ...
ListNode producerTwo = ... get producer data ...

var initialElements = new List<(ListNode Element, int Priority)>();

// Limit the queue to the first 200 inputs from the producers
while (initialElements.Count <= 200 
    && (producerOne is not null || producerTwo is not null))
{
    // build initial elements, limited to the first 200 elements 
}

var priorityQueue = new PriorityQueue<ListNode, int>(initialElements);

while (priorityQueue.TryPeek(out var peekedElement, out _))
{
    // Each time we dequeue an element and have a next element to enqueue 
    // we want to use DequeueEnqueue which gives better performance than a Dequeue() followed by a sequential Enqueue()
    var minElement = peekedElement.Next != null
        ? priorityQueue.DequeueEnqueue(peekedElement.Next, peekedElement.Next.Value)
        : priorityQueue.Dequeue();
        
    // ... consumer logic that uses minElement ....
}

@vslee
Copy link

vslee commented Sep 7, 2022

Should we also have a TryDequeueEnqueue variant?

This method seems redundant. A queue.Count > 0 check should suffice to verify that the operation will succeed.

What if the contents of the queue changes between the queue.Count > 0 check and the calling of DequeueEnqueue()?

@stephentoub
Copy link
Member

What if the contents of the queue changes between the queue.Count > 0 check and the calling of DequeueEnqueue()?

Then a non-thread-safe collection is being mutated concurrently erroneously.

@RyanThomas73
Copy link
Contributor Author

@krwq
Should I create a PR now or wait until the API proposal review has happened first?

@eiriktsarpalis
I was looking through the micro-benchmarks to see if there were PriorityQueue benchmarks for EnqueueDequeue(..); if so I figured I'd add corresponding benchmarks for the inverse DequeueEnqueue(..).

I was hoping you could clarify the intention of this benchmark:
https://github.com/dotnet/performance/blob/b905d2cc8fed5e40c5e2789365aab5f7c8eb5d38/src/benchmarks/micro/libraries/System.Collections/PriorityQueue/Perf_PriorityQueue.cs#L57.

Do you know if it is/was intended to serve as a sequential operations comparison for the K_Max_Elements() benchmark? If so should the sequential operations benchmark be updated to have the same fixed size k and loop/iterator ranges? Would there be value in moving the fixed queue size benchmarks into a separate benchmark class and defining the sequential dequeue/enqueue as the benchmark baseline?

@eiriktsarpalis
Copy link
Member

Should I create a PR now or wait until the API proposal review has happened first?

Please wait until and if the API proposal has been approved.

Do you know if it is/was intended to serve as a sequential operations comparison for the K_Max_Elements() benchmark? If so should the sequential operations benchmark be updated to have the same fixed size k and loop/iterator ranges?

The two benchmarks are independent and aren't intended for direct comparison. The purpose of these benchmarks is detecting possible performance regressions, so we generally try to avoid changing them once they have been added.

@bartonjs
Copy link
Member

bartonjs commented Sep 20, 2022

Video

  • We're unclear on how useful this method would be, but assuming there's value it looks good as proposed.
namespace System.Collections.Generic;

public class PriorityQueue<TElement, TPriority>
{
    public TElement DequeueEnqueue(TElement element, TPriority priority);
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Sep 20, 2022
@eiriktsarpalis
Copy link
Member

@RyanThomas73 API has been approved as proposed, feel free to open a PR contributing the implementation.

RyanThomas73 added a commit to RyanThomas73/dotnet-runtime that referenced this issue Sep 21, 2022
…tions.Generic.PriorityQueue public api and unit test cases for the new method
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Sep 21, 2022
RyanThomas73 added a commit to RyanThomas73/dotnet-runtime that referenced this issue Sep 22, 2022
…property test case and update DequeueEnqueue test assertion and xunit attribute
RyanThomas73 added a commit to RyanThomas73/dotnet-runtime that referenced this issue Sep 22, 2022
…tyQueue.PropertyTests.cs test case method names
eiriktsarpalis pushed a commit that referenced this issue Sep 29, 2022
* Issue #75070 - Add a DequeueEnqueue method to the System.Collections.Generic.PriorityQueue public api and unit test cases for the new method

* Issue #75070 - Remove unnecessary PriorityQueue DequeueEnqueue property test case and update DequeueEnqueue test assertion and xunit attribute

* Item #75070 - Revert the unnecessary name changes to the PriorityQueue.PropertyTests.cs test case method names
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 29, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Collections
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants