RFR: 8244551: Shenandoah: Fix racy update of update_watermark

[Zhengyu Gu]

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

-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
>