RFR: 8221723: Avoid storing zero to String.hash

[Claes Redestad]

Hi Peter,

first: neat trick!

On 2019-04-02 12:56, Peter Levart wrote:
> Hi Claes,
> 
> I also think that the variant shown below should be compatible with 
> existing VM code that "injects" hash value. No changes necessary, right?

Kind of. If an injector forgets setting the hashIsZero bit, it'd just
be set the first time someone calls hashCode, which is a small
performance penalty in general, but a more real problem for the archive
use case since we want to avoid any stores to the String object.

Regardless, I think it'd be straightforward to update the hash injectors
to do the right thing, especially now that we have a protocol that don't
need any fences. I'll file an RFE and probably start working on this
right away.

Thanks!

/Claes

> 
> Peter
> 
> On 4/2/19 12:53 PM, Peter Levart wrote:
>> Hi Claes,
>>
>> On 4/2/19 10:29 AM, Claes Redestad wrote:
>>> Hi Peter,
>>>
>>> On 2019-04-01 23:54, Peter Levart wrote:
>>>>
>>>> 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. :-)
>>
>> Right. And after I posted this message yesterday, it occurred to me 
>> that it is possible to get away without fences if this additional 
>> boolean flag is not called "hashCalculated" but "hashIsZero". Like in 
>> the following example:
>>
>>     private int hash;
>>     private boolean hashIsZero;
>>
>>     public int hashCode() {
>>         int h = hash;
>>         if (h == 0 && !hashIsZero) {
>>             h = isLatin1() ? StringLatin1.hashCode(value)
>>                            : StringUTF16.hashCode(value);
>>             if (h == 0) {
>>                 hashIsZero = true;
>>             } else {
>>                 hash = h;
>>             }
>>         }
>>         return h;
>>     }
>>
>> The trick here is that the logic writes to either 'hashIsZero' or to 
>> 'hash' field but never to both, so there's no ordering to be performed...
>>
>> Regards, Peter