Re[2]: RFR: 8226297: Dual-pivot quicksort improvements

[Vladimir Yaroslavskiy]

Hi Brent,

Thank you for review, see my comments inline:

>Oct 8, 2019, 1:53 +03:00 от Brent Christian <brent.christian at oracle.com>:
>
>Hi, Vladimir
>
>I've read through the changes[1].  I have the following comments:
>
>---
>src/java.base/share/classes/java/util/Arrays.java
>
>* Regarding the indentation changes in the equals() methods:
>
>-     * <pre>    {@code new Double(d1).equals(new Double(d2))}</pre>
>+     * <pre>{@code new Double(d1).equals(new Double(d2))}</pre>
>...
>-     * <pre>    {@code new Double(d1).equals(new Double(d2))}</pre>
>+     * <pre>{@code new Double(d1).equals(new Double(d2))}</pre>
>...
>-     * <pre>    {@code new Float(f1).equals(new Float(f2))}</pre>
>+     * <pre>{@code new Float(f1).equals(new Float(f2))}</pre>
>...
>-     * <pre>    {@code new Float(f1).equals(new Float(f2))}</pre>
>+     * <pre>{@code new Float(f1).equals(new Float(f2))}</pre>
>
>Looking at the generated JavaDoc, this indentation is intentional.  I've 
>reverted these changes.
>
>* Otherwise, the JavaDoc changes are limited to within @implNote 
>(previously written out as "Implementation note", in some cases).  I 
>don't believe a CSR is warranted.

Agree, thank you for reverting these changes.

>---
>test/jdk/java/util/Arrays/Sorting.java
>
>Some general comments:
>
>* I see a lot of renames[2] that I don't think add value, and clutter 
>the changes (1000+ total changed lines in a 2000 line file).  I feel 
>similarly about the changes from post- to pre- incrementing of the for() 
>loop indices.  If it's OK with you, Vladimir, I can take care of 
>reverting these changes.

The following renaming changes are important
and should be kept. My explanation is here:

* testAndCheck -> testQuicksort

Quicksort only was tested in previous version
(parallel Quicksort was tested in other class).
Now all tests are common for parallel and sequential
Quicksorts, plus tests for heap sort are also added.
Therefore, testAndCheck is spitted into two methods:
testQuicksort and testHeapSort.

* Merge -> Merging [Sort|Builder]

Before starting Quicksort we check if given array is
almost sorted. In this case sorted sub-parts are merged.
It is not the same as merge sort, therefore I use proper
name "merging" instead of "merge".

* ourDescription -> name, myKey -> key,
myValue -> value, MyRandom -> TestRandom

There was practice to use prefix "my" for
non-static fields of a class, and "our" for static
fields. Now this approach is not actual because it
is easy to recognize the type of a field in modern IDE
the names are shorter.

* seed -> random

I renamed to have consistent names "random : randoms"
in for-loop instead of "seed : randoms", see "for (long random : randoms)"
----------------

* Other changes, like failed -> fail, i++ -> ++i,
and TypeConverter, FloatBuilder, DoubleBuilder, SortedBuilder
to be non-static can be reverted.

I can revert them, if you agree.

>* I like that the various DualPivotQuickSort methods are tested 
>directly (via SortingHelper).  I'd prefer that we also continue testing 
>direct calls to Arrays.sort(), as that's what users will call.  What 
>would be the best way to add that back in?  (Maybe a SortingHelper 
>variant that calls Arrays.sort()?).
I will add direct calls to Arrays.sort() and Arrays.parallelSort() also.

>* Having the test operation depend on setting various values to the 
>static 'sortingHelper' field seems brittle to me.  This could be a good 
>opportunity to convert from static to instance methods, make 
>'sortingHelper' and 'name' member variables, and create separate Sorting 
>objects for each SortingHelper. Such a refactoring is not strictly 
>necessary, but I think it would be nice to have.

Good idea, I will implement it and send you update version very soon.

>---
>test/jdk/java/util/Arrays/FailedFloat.java
>
>Is this test necessary, given the addition of testFloatNegativeZero() in 
>Sorting.java ?

Not necessary, because testFloatNegativeZero() covers this case.
This class was created to reproduce bug when float-pointing zeros
and NaNs are sorted.

Please remove FailedFloat.java. It makes sense to add the source of this class to 8226297.

>Thanks,
>-Brent
>
>1. 
>http://cr.openjdk.java.net/~bchristi/8226297/webrev05-adaptive/webrev/index.html
>
>2. Name changes in Sorting.java:
>     testAndCheck -> testQuicksort
>     seed -> random
>     ourDescription -> name
>     failedSort -> failSort
>     failed -> fail
>     myKey -> key
>     myValue -> value
>     Merge->Merging [Sort|Builder]
>     MyRandom->TestRandom
>     failedCompare->failCompare
>     change TypeConverter, FloatBuilder, DoubleBuilder, SortedBuilder to 
>be non-static
Thank you,
Vladimir