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

[coleen.phillimore at oracle.com]

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();
>
> 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();

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