RFR: 8244551: Shenandoah: Fix racy update of update_watermark

Zhengyu Gu zgu at redhat.com
Thu May 7 12:55:02 UTC 2020


I did not follow this _update_watermark stuff closely. My experiment 
with Atomic::load/store(_update_watermark), I still saw stalled values. 
So I am not sure load_acquire/release_store() fix the stalled value 
problem, or stalled value just means more work, but not correctness?

-Zhengyu

On 5/7/20 8:48 AM, Roman Kennke wrote:
>> 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
>>>
>>
> 




More information about the hotspot-gc-dev mailing list