feat: #954 - TryDequeue and TryPeek Queue extension methods #957

devklick · 2024-08-16T12:27:58Z

You've read the Contributor Guide and Code of Conduct.
You've included unit or integration tests for your change, where applicable.
You've included inline docs for your change, where applicable.
There's an open issue for the PR that you are making. If you'd like to propose a change, please open an issue to discuss the change or find an existing issue.

Add TryDequeue and TryPeek Queue extension methods

Fixes #954

Description

New class created, QueueExtensions, with two new methods, TryDequeue and TryPeek. These are compiled based on the FEATURE_QUEUE_TRYDEQUEUE_TRYPEEK flag.

Inline documentation included
Unit tests included

Notes:

One of the requirements on the linked ticket was to update all references to use these new functions. I have updated some of them, only where it will eliminate a check along the lines of queue.count > 0 && queue.Dequeue/Peek(). There are more calls to queue.Dequeue() and queue.Peek() where it did not seem suitable to replace with the Try* equivalent, so I have left them as-is.

The QueueExtensionsTests suite of tests will be testing the actual methods on the Queue object when testing on any framework other than netstandard2.0 and .NET Framework, rather than testing our extensions. Should these tests be configured to only run for certain frameworks? Seems redundant to have unit tests targeting a built-in feature.

NightOwl888

Thanks for the PR and your willingness to help make our project better.

There are a few changes we would like you to make before we can accept this PR. First, please see the comments inline.

NightOwl888 · 2024-08-17T01:29:15Z

src/Lucene.Net.Analysis.Common/Analysis/Shingle/ShingleFilter.cs

-            {
-                firstToken = inputWindow.Dequeue();
-            }
+            inputWindow.TryDequeue(out InputWindowToken firstToken);


https://github.com/apache/lucene/blob/releases/lucene-solr/4.8.1/lucene/analysis/common/src/java/org/apache/lucene/analysis/shingle/ShingleFilter.java#L454-L457

Since this doesn't align very well with the Java source, lets add a comment so it is clear that firstToken will be null if the queue is empty.

inputWindow.TryDequeue(out InputWindowToken firstToken); // LUCENENET: firstToken will be null if the queue is empty

NightOwl888 · 2024-08-17T01:41:20Z

src/Lucene.Net.Analysis.Phonetic/DoubleMetaphoneFilter.cs

@@ -52,12 +53,11 @@ public DoubleMetaphoneFilter(TokenStream input, int maxCodeLength, bool inject)

        public override bool IncrementToken()
        {
-            for (;;)
+            for (; ; )


As much as I dislike using a for loop this way, it is what was done in Java.

https://github.com/apache/lucene/blob/releases/lucene-solr/4.8.1/lucene/analysis/phonetic/src/java/org/apache/lucene/analysis/phonetic/DoubleMetaphoneFilter.java#L53

But the original source didn't have spaces. So could we please remove them again?

Apologies - automatic editor linting. I've removed those spaces.

NightOwl888 · 2024-08-17T01:55:52Z

src/Lucene.Net.Tests/Support/QueueExtensionsTests.cs

+        {
+            Queue<int> queue = null;
+            var ex = Assert.Throws<ArgumentNullException>(() => queue.TryDequeue(out int _));
+            Assert.That(ex.ParamName, Is.EqualTo("queue"));


Assert.That() is painfully slow. In fact, we don't use any NUnit asserts because they cause our tests to take about twice as long. Please add the following alias after the list of usings:

using Assert = Lucene.Net.TestFramework.Assert;

And change each Assert.That() in this file to use Assert.AreEqual(). We have optimized overloads of these asserts that accept value types.

Apologies, I should have checked the existing tests to see what to use. I've updated to use Lucene Assert now.

Note that since the Lucene equivalent doesnt cast the returned exception to T (which I guess might be for performance reasons), I'm casting it to ArgumentNullException within two of these tests to verify the ParamName. Please let me know if I should avoid doing do. I notice that the existing tests for Assert.Throws<ArgumentNullException> dont bother checking the ParamName, so maybe this is overkill?

Yeah, usually just asserting the correct error type is enough, although some tests check error messages, as well.

NightOwl888 · 2024-08-17T02:40:54Z

src/Lucene.Net/Index/IndexWriter.cs

@@ -2690,17 +2690,12 @@ public virtual MergePolicy.OneMerge NextMerge()
            UninterruptableMonitor.Enter(this);
            try
            {
-                if (pendingMerges.Count == 0)


I believe in this case, this is a performance optimization to give up the lock as soon as possible on an empty queue.

https://github.com/apache/lucene/blob/releases/lucene-solr/4.8.0/lucene/core/src/java/org/apache/lucene/index/IndexWriter.java#L2047-L2056

Please revert the changes in this method, as it is easier to follow as-is.

Also, please add a comment after the method declaration:

public virtual MergePolicy.OneMerge NextMerge() // LUCENENET TODO: API - Revert name to GetNextMerge() to match Java

We will need to review more of the API before renaming to make certain that it is consistent with other method names.

NightOwl888 · 2024-08-17T02:47:17Z

src/Lucene.Net/Search/NumericRangeQuery.cs

@@ -487,12 +488,12 @@ protected override sealed AcceptStatus Accept(BytesRef term)
            {
                while (currentUpperBound is null || termComp.Compare(term, currentUpperBound) > 0)
                {
-                    if (rangeBounds.Count == 0)
+                    if (!rangeBounds.TryPeek(out BytesRef rangeBound))


This appears to be a performance optimization to exit the loop as soon as possible.

https://github.com/apache/lucene/blob/releases/lucene-solr/4.8.0/lucene/core/src/java/org/apache/lucene/search/NumericRangeQuery.java#L520

Please revert the changes to this method.

NightOwl888 · 2024-08-17T02:53:17Z

src/Lucene.Net/Support/QueueExtensions.cs

+        /// <param name="result">The removed object</param>
+        /// <returns><c>true</c> if the object was successfully removed; <c>false</c> if the <see cref="Queue{T}"/> is empty.</returns>
+        /// <exception cref="ArgumentNullException"></exception>
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]


I believe this is a bit too much code to make inlining worth it. It will only apply to legacy frameworks, anyway. Please remove [MethodImpl(MethodImplOptions.AggressiveInlining)] both here and on TryPeek().

NightOwl888 · 2024-08-17T03:27:48Z

src/Lucene.Net/Support/QueueExtensions.cs

+        /// <param name="queue">The <see cref="Queue{T}"/> to be checked</param>
+        /// <param name="result">The removed object</param>
+        /// <returns><c>true</c> if the object was successfully removed; <c>false</c> if the <see cref="Queue{T}"/> is empty.</returns>
+        /// <exception cref="ArgumentNullException"></exception>


Please add " is null</>." within this element both here and in TryPeek().

Hi @NightOwl888, can you please clarify, do you mean the following?

<exception cref="ArgumentNullException">is null</exception>

My apologies. GitHub stripped the angle brackets.

<exception cref="ArgumentNullException"><paramref name="queue"/> is <c>null</>.</exception>

NightOwl888

I apologize for adding additional requirements, but could you please add nullable reference type support? See my inline comments.

src/Lucene.Net/Support/QueueExtensions.cs

NightOwl888 · 2024-08-17T10:48:54Z

src/Lucene.Net/Support/QueueExtensions.cs

+        /// <param name="result">The removed object</param>
+        /// <returns><c>true</c> if the object was successfully removed; <c>false</c> if the <see cref="Queue{T}"/> is empty.</returns>
+        /// <exception cref="ArgumentNullException"><paramref name="queue"/> is <c>null</c>.</exception>
+        public static bool TryDequeue<T>(this Queue<T> queue, out T result)


Please add the [MaybeNullWhen(false)] attribute before the out modifier.

NightOwl888 · 2024-08-17T10:49:07Z

src/Lucene.Net/Support/QueueExtensions.cs

+        /// <param name="result">If present, the object at the beginning of the <see cref="Queue{T}"/>; otherwise, the default value of <typeparamref name="T"/></param>
+        /// <returns><c>true</c> if there is an object at the beginning of the <see cref="Queue{T}"/>; <c>false</c> if the <see cref="Queue{T}"/> is empty.</returns>
+        /// <exception cref="ArgumentNullException"><paramref name="queue"/> is <c>null</c>.</exception>
+        public static bool TryPeek<T>(this Queue<T> queue, out T result)


Please add the [MaybeNullWhen(false)] attribute before the out modifier.

NightOwl888 · 2024-08-18T13:58:08Z

Thanks again for your contribution. Your help is appreciated.

devklick added 4 commits August 16, 2024 13:26

feat: apache#954 - Add TryDequeue and TryPeek Queue extension methods

26e22e1

feat: apache#954 - Use TryDequeue and TryPeek Queue extension methods

7bd43ab

Add license

d4d8863

Use Queue.TryDequeue where possible

5f0b062

devklick marked this pull request as draft August 16, 2024 17:17

devklick added 3 commits August 16, 2024 18:27

Use Queue.TryPeek where possible

3df26ff

Use explicit type over var

264fb7c

Fix tests

7c14ab7

devklick marked this pull request as ready for review August 16, 2024 17:51

Run tests depending on feature flag

842727b

NightOwl888 requested changes Aug 17, 2024

View reviewed changes

NightOwl888 reviewed Aug 17, 2024

View reviewed changes

devklick added 3 commits August 17, 2024 09:02

Reinstate previous performance optimizations

603ead3

Fix QueueExtensions exception docs

c72626b

Remove unnecessary ParamName assertion in QueueExtensionsTests

66e7e53

NightOwl888 requested changes Aug 17, 2024

View reviewed changes

Add nullable support to QueueExtensions

d5f79d3

NightOwl888 approved these changes Aug 18, 2024

View reviewed changes

NightOwl888 merged commit 0d7bba7 into apache:master Aug 18, 2024
199 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: #954 - TryDequeue and TryPeek Queue extension methods #957

feat: #954 - TryDequeue and TryPeek Queue extension methods #957

devklick commented Aug 16, 2024 •

edited

Loading

NightOwl888 left a comment

NightOwl888 Aug 17, 2024

NightOwl888 Aug 17, 2024

devklick Aug 17, 2024

NightOwl888 Aug 17, 2024

devklick Aug 17, 2024

NightOwl888 Aug 17, 2024

NightOwl888 Aug 17, 2024

NightOwl888 Aug 17, 2024

NightOwl888 Aug 17, 2024

NightOwl888 Aug 17, 2024

devklick Aug 17, 2024

NightOwl888 Aug 17, 2024

NightOwl888 left a comment

NightOwl888 Aug 17, 2024

NightOwl888 Aug 17, 2024

NightOwl888 commented Aug 18, 2024

feat: #954 - TryDequeue and TryPeek Queue extension methods #957

feat: #954 - TryDequeue and TryPeek Queue extension methods #957

Conversation

devklick commented Aug 16, 2024 • edited Loading

Description

NightOwl888 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NightOwl888 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NightOwl888 commented Aug 18, 2024

devklick commented Aug 16, 2024 •

edited

Loading