RFR: 8244551: Shenandoah: Fix racy update of update_watermark

[Roman Kennke]

> So, the patch fixes the ordering of _top and _update_watermark to avoid
> assertion failure, but adds unnecessary penalty to production build?

I wouldn't call that unnecessary. And the cost of it seems ok too, we
don't update so very often, and we don't query it so very often either.
I made the update-at-safepoint not use barriers, so that critical path
should be ok.

Roman


> -Zhengyu
> 
> 
> On 5/7/20 7:37 AM, Roman Kennke wrote:
>>>>> *) What's the point of this cast?
>>>>>
>>>>>   133   *const_cast<HeapWord**>(&_update_watermark) =  w;
>>>>
>>>> Make access non-volatile. You think it's too much?
>>>
>>> I don't think that is relevant. As long as it is not the atomic store
>>> (with fences), we are fine.
>>
>> Ok. I will push the fix with this additional change then:
>>
>> diff -r 7910475d5001
>> src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.inline.hpp
>> --- a/src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.inline.hpp
>> Thu May 07 07:03:25 2020 -0400
>> +++ b/src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.inline.hpp
>> Thu May 07 07:36:26 2020 -0400
>> @@ -128,9 +128,8 @@
>>   // Fast version that avoids synchronization, only to be used at
>> safepoints.
>>   inline void
>> ShenandoahHeapRegion::set_update_watermark_at_safepoint(HeapWord* w) {
>>     assert(bottom() <= w && w <= top(), "within bounds");
>> -  assert(SafepointSynchronize::is_at_safepoint(),
>> -         "only call set_update_watermark_at_safepoint at safepoint");
>> -  *const_cast<HeapWord**>(&_update_watermark) =  w;
>> +  assert(SafepointSynchronize::is_at_safepoint(), "Should be at
>> Shenandoah safepoint");
>> +  _update_watermark = w;
>>   }
>>
>>   #endif // SHARE_GC_SHENANDOAH_SHENANDOAHHEAPREGION_INLINE_HPP
>>
>>
>> Thanks for reviewing!
>> Roman
>>
>