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 >
