RFR (S) 8206471: Race with ConcurrentHashTable deleting items on insert with cleanup thread

[coleen.phillimore at oracle.com]

On 7/9/18 7:32 AM, coleen.phillimore at oracle.com wrote:
>
> On 7/8/18 9:20 PM, David Holmes wrote:
>> Hi Coleen,
>>
>> On 7/07/2018 5:41 AM, coleen.phillimore at oracle.com wrote:
>>> Summary: Only fetch Node::next once and use that result.
>>>
>>> A racing thread could NULL next->next()->next().  The Node itself is 
>>> stable until the write_synchronize() but the pointers may be 
>>> updated. See bug for more detail.
>>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/8206471.01/webrev
>>> bug link https://bugs.openjdk.java.net/browse/JDK-8206471
>>
>> The change looks good.
>>
>> Could there be a similar race at:
>>
>>  552       bucket->release_assign_node_ptr(rem_n_prev, rem_n->next());
>>  553       rem_n = rem_n->next();
>>

Probably not because this instance has the bucket lock, but I'll change 
it anyway.

>> Even if not, it is marginally more performant to only do the 
>> load-acquire once.
>>
>> Similarly:
>>
>>  663 new_table->get_bucket(odd_index)->release_assign_node_ptr(odd,
>>  664 aux->next());
>>  665 new_table->get_bucket(even_index)->release_assign_node_ptr(even,
>>  666 aux->next());
>>
>> combined with:
>>
>>  685     aux = aux->next();
>>
>> makes for 3 load-acquire (and 2 if we take the else at line #675).
>>
>> And again:
>>
>>  982       bucket->release_assign_node_ptr(rem_n_prev, rem_n->next());
>>  983       rem_n = rem_n->next();
>
I believe the value of next() is stable in all these cases, and it's 
fine to only load once.

open webrev at http://cr.openjdk.java.net/~coleenp/8206471.02/webrev

Ran the extensive gtests that Robbin wrote to cover zipping and 
unzipping the hashtable and rerunning hs-tier1,2.

Thanks,
Coleen

> Thank you for noticing these other double loads.  I'll study them to 
> see if there's a race or see if there's a reason they have double 
> loads, but I'll change them unless there is a reason not to.
>
> Thanks!
> Coleen
>
>>
>> Thanks,
>> David
>> -----
>>
>>> Tested with SymbolTable changes and tests that failed.  Also tested 
>>> with mach5 hs-tier1-5 (in progress).
>>>
>>> This is actually Robbin's fix, and my review is that it looks good.
>>>
>>> Thanks,
>>> Coleen
>