It's interesting that this version performs as good or *better* than
value.length > 0 check. Intuitively, value.length is going to be used
anyway (if h == 0) in the loop and so there should be no penalty of loading
and branching on it (given the change here has a branch anyway) and
compiler can keep that value in a register for the actual loop.
Looking at the generated asm for current String.hashCode(), it doesn't look
like compiler is actually using the user-written check to skip its own
check (see the two tests at the bottom of this snippet):
0x00007f7e312a0d8c: mov %rsi,%rbx
0x00007f7e312a0d8f: mov 0x10(%rsi),%eax ;*getfield hash
; -
java.lang.String::hashCode@1 (line 1454)
0x00007f7e312a0d92: test %eax,%eax
0x00007f7e312a0d94: jne 0x00007f7e312a0e85 ;*ifne
; -
java.lang.String::hashCode@6 (line 1455)
0x00007f7e312a0d9a: mov 0xc(%rsi),%ebp ;*getfield value
; -
java.lang.String::hashCode@10 (line 1455)
0x00007f7e312a0d9d: mov 0xc(%r12,%rbp,8),%r10d ;*arraylength
; -
java.lang.String::hashCode@13 (line 1455)
; implicit exception:
dispatches to 0x00007f7e312a0ea9
0x00007f7e312a0da2: test %r10d,%r10d
0x00007f7e312a0da5: jle 0x00007f7e312a0e91 ;*ifle
; -
java.lang.String::hashCode@14 (line 1455)
0x00007f7e312a0dab: test %r10d,%r10d
0x00007f7e312a0dae: jbe 0x00007f7e312a0e95
It does however continue using r10.
On Wed, May 13, 2015 at 8:36 AM, Sergey Bylokhov <sergey.bylok...@oracle.com
> wrote:
> Just curious, what is the difference between this fix and an old version
> of the code:
>
> http://cr.openjdk.java.net/~shade/8058643/webrev.01/src/java.base/share/classes/java/lang/String.java.sdiff.html
>
> I meant:
> "if (h == 0 && value.length > 0) {}"
> vs
> "if (h != 0) {hash = h;}"
>
>
> On 13.05.15 14:51, Claes Redestad wrote:
>
>> Hi,
>>
>> 9-b33 introduced a sustained regression in SPECjvm2008
>> xml.transform on a number of our test setups.
>>
>> JDK-8058643 removed the check on value.length > 0, which
>> means repeated calls to "".hashCode() now do a store of the
>> calculated value (0) to the hash field. This has potential to
>> cause excessive cache coherence traffic in xml.transform[1]
>>
>> Extracting the code that showed up in profiles to a micro[2] and
>> running this in multiple threads is up to 100x slower in 9-b33 on
>> a dual-socket machine.
>>
>> Adding back the check that value.length > 0 before entering the
>> calculation loop recuperates the loss in both micro and
>> xml.transform, but we're seeing as good or better number by
>> simply guarding the store:
>>
>> Webrev: http://cr.openjdk.java.net/~redestad/8061254/webrev.00/
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8061254 (confidential
>> due to links to internal systems, sorry!)
>>
>> This additionally avoids the field store (and potential for pointless
>> cache traffic) also on non-empty strings whose hash value happen
>> to equals 0.
>>
>> Thanks!
>>
>> /Claes
>>
>> [1] See
>> com.sun.org.apache.xml.internal.dtm.ref.ExpandedNameTable#getExpandedTypeID.
>> [2] http://cr.openjdk.java.net/~redestad/8061254/expandedtypeid-micro.zip
>>
>
>
> --
> Best regards, Sergey.
>
>
