RFR [9] 8151384: Examine sun.misc.ASCIICaseInsensitiveComparator

[Xueming Shen]

Chris,

515                 hashCode = name.toLowerCase(Locale.ROOT).hashCode();

otherwise  + 1

-sherman

On 03/07/2016 01:55 PM, Chris Hegarty wrote:
> Aleksey,
>
> Very helpful, as always.
>
> I pushed the methods down into  String[Latin1|UTF16], and followed existing
> style. This is much cleaner.
>
> Thanks for catching the silly mistakes in the benchmarks.
>
> Updated links:
>    http://cr.openjdk.java.net/~chegar/8151384/webrev.01/
>    http://cr.openjdk.java.net/~chegar/8151384/bench.01/
>
> -Chris.
>
>
> On 7 Mar 2016, at 16:51, Aleksey Shipilev<aleksey.shipilev at oracle.com>  wrote:
>
>> Hi,
>>
>> On 03/07/2016 07:29 PM, Chris Hegarty wrote:
>>> What is in the webrev is specialized versions of compare when
>>> the coder of the strings match. Alternatively, this could be pushed
>>> down to String[Latin1|UTF16].
>>>
>>> Webrev&  bug:
>>>   http://cr.openjdk.java.net/~chegar/8151384/webrev.00/
>>>   https://bugs.openjdk.java.net/browse/JDK-8151384
>> Overall, good cleanup idea. I think the actual helpers deserve to be
>> pushed to String[Latin1|UTF16], as String is already overloaded with
>> lots of code. See how e.g. String.regionMatches(boolean ignoreCase, int
>> toffset, String other, int ooffset, int len) does it.
>>
>> Nits:
>>
>> *) Change:  compareLatin1ToUTF16(v2, v1) * -1;
>>         To: -compareLatin1ToUTF16(v2, v1);
>>
>> *) Do we really need to cast up/down to "char" in compareLatin1*?
>>
>>
>>> Benchmarks and results ( based, somewhat, on Aleksey's [1] ):
>>>   http://cr.openjdk.java.net/~chegar/8151384/bench/
>> Comments on benchmarks (that might have impact on validity):
>>
>> *) "# JMH 1.6 (released 388 days ago, please consider updating!)"
>>
>> *) CaseInsensitiveCompare.cmp1_cmp1 is suspiciously unaffected by size.
>> That's because benchmark goes through the identity comparison:
>>
>>     @Benchmark
>>     @CompilerControl(CompilerControl.Mode.DONT_INLINE)
>>     public int cmp1_cmp1() {
>>         return CASE_INSENSITIVE_ORDER.compare(cmp1_1, cmp1_1);
>>     }
>>
>> ...should really be:
>>
>>     @Benchmark
>>     @CompilerControl(CompilerControl.Mode.DONT_INLINE)
>>     public int cmp1_cmp1() {
>>         return CASE_INSENSITIVE_ORDER.compare(cmp1_1, cmp1_2);
>>     }
>>
>> *) Probable dead-code elimination here:
>>
>>     @Benchmark
>>     @CompilerControl(CompilerControl.Mode.DONT_INLINE)
>>     public void StringCaseInsensitiveComparator() {
>>         List<String>  strings = AVAILABLE_CHARSETS;
>>         for (String s1 : strings) {
>>             for (String s2 : strings) {
>>                  String.CASE_INSENSITIVE_ORDER.compare(s1, s2);
>>             }
>>         }
>>     }
>>
>> ...should be:
>>
>>     @Benchmark
>>     @CompilerControl(CompilerControl.Mode.DONT_INLINE)
>>     public void StringCaseInsensitiveComparator(Blackhole bh) {
>>         List<String>  strings = AVAILABLE_CHARSETS;
>>         for (String s1 : strings) {
>>             for (String s2 : strings) {
>>                  bh.consume(String.CASE_INSENSITIVE_ORDER.compare(s1, s2));
>>             }
>>         }
>>     }
>>
>>
>> Thanks,
>> -Aleksey
>>