RFR: 8226297: Dual-pivot quicksort improvements

[Brent Christian]

Hi,

I've created a webrev of the latest test changes that Vladimir sent me:

http://cr.openjdk.java.net/~bchristi/8226297/webrev09-testUpdate/

(I also created an incremental webrev[1] along the way, in case that's 
helpful to anyone).

I have just a few comments:

test/jdk/java/util/Arrays/Sorting.java

* I found some instances of "FloatPointing", which I've already changed 
to "FloatingPoint". :)

--

      // Array lengths used in a long run (default)
      private static final int[] LONG_RUN_LENGTHS = {
-        1, 2, 3, 5, 8, 13, 21, 34, 55, 100, 1000, 10000, 100000, 1000000 };
+        1, 3, 8, 21, 55, 100, 1_000, 10_000, 100_000 };

      // Array lengths used in a short run
      private static final int[] SHORT_RUN_LENGTHS = {
-        1, 2, 3, 21, 55, 1000, 10000 };
+        1, 8, 55, 100, 10_000 };

Vladimir, can you tell me why some of the length values have been removed?

--

1570     private static enum TypeConverter {
...
1637         abstract void convert(Object src, Object dst);

The 'src' argument to convert is always cast to an int[].  Can it be 
made an int[] in the method signature, instead of Object ?  I can make 
the change if that sounds good.

Other than those, this is pretty much ready to go.

Thanks,
-Brent

1. http://cr.openjdk.java.net/~bchristi/8226297/webrev09-incremental/

On 10/8/19 5:41 PM, Brent Christian wrote:
> Hi, Vladimir
> 
> On 10/8/19 3:18 PM, Vladimir Yaroslavskiy wrote:
>>
>> The following renaming changes are important
>> and should be kept. My explanation is here
>  > ...
> 
> Thanks for the explanation - much appreciated.
> 
>> * 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.
> 
> That would be great, thanks.
> 
>>     * 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.
> 
> Great, thank you.
> 
>>     * 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.
> 
> Sounds good!
> 
>>>     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.
> 
> Done and done.
> 
> -Brent