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.shipi...@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


Reply via email to