RFR: 8221723: Avoid storing zero to String.hash

[Claes Redestad]

Hi Peter,

On 2019-04-01 23:54, Peter Levart wrote:
> 
> On 4/1/19 10:44 PM, Claes Redestad wrote:
>> We actually looked at this idea earlier today, and squeezing a "not-
>> computed" value into String might be "free" since there seems to be a
>> padding gap on all VM varieties (at least according to JOL estimates[1])
>>
>> That'd be a larger endeavor, though, since there are places in VM that
>> calculates and injects the String.hash value.
> 
> In addition, this might not be easy from another point of view. You all 
> know that there's no synchronization performed when caching the hash 
> value. This works, because 32 bits are always read or written 
> atomically. If there was another byte to be read or written together 
> with 32 bit value, it would be tricky to get the ordering right and 
> still keep the performance.

I won't do this for the current RFE, but it's an interesting topic for
a future one. :-)

Disclaimer: As I'm trying to be clever about concurrency I'm likely
missing something...

... but I think a couple of barriers would be enough to ensure
sufficient visibility/atomicity for this case, since the only
catastrophic situation would be if another thread observes hash = 0 and
hashCalculated = true when the real hash value is != 0.

     private boolean hashCalculated;
     private static Unsafe U = Unsafe.getUnsafe();

     public int hashCode() {
         int h = hash;
         if (h == 0 && !hashCalculated) {
             if (!hashCalculated) {
                 hash = h = isLatin1() ? StringLatin1.hashCode(value)
                         : StringUTF16.hashCode(value);
                 // Ensure the store of the hashCalculated value is not
                 // reordered with the store of the hash value
                 U.storeStoreFence();
                 hashCalculated = true;
             } else {
                 // We could have lost a race where we read hash = 0,
                 // then another thread stored hash and hashCalculated,
                 // then we read hashCalculated = true when the real hash
                 // value is not 0.
                 // A re-read behind a load fence should be sufficient
                 U.loadLoadFence();
                 h = hash;
             }
         }
         return h;
     }

This is performance neutral on the fast-path (hash is calculated and
non-zero), and the extra fences cost ~1.2ns/op on my machine for cases
where the calculated hash code is 0, which is more or less the same
overhead as the extra branches we have now. The benefit of course is
that for non-empty Strings with hash 0 then repeated hashCode()
invocation is now as fast as "".hashCode()

(One performance risk is that the added calls to U.*Fence will mess up
inlining heuristics.)

Thanks!

/Claes