RFR: Remove racy assert in ShenandoahResetNextMarkBitmapTraversalTask

[Aleksey Shipilev]

On 09/11/2018 03:27 PM, Zhengyu Gu wrote:
> On 09/11/2018 08:06 AM, Roman Kennke wrote:
>> The assert in ShenandoahResetNextMarkBitmapTraversalTask that is racily
>> checking TAMS against top. Racy because other manipulations (e.g.
>> recycle-assist) happen under HeapLock, but not this check. This
>> sometimes blows up. I don't know how to fix that except removing it. We
>> could acquire the HeapLock under #ifdef ASSERT but that seems like overkill.
>>
>>
> Hmmm... I am not sure this ShenandoahResetNextBitmapTraversalTask is safe at all.
> 
> The region status races against mutator's allocation, e.g. region->is_trash() can race against
> FreeSet's recycle_trash(), now you have memory ordering issues to worry about. Am I wrong?

I am with Zhengyu here: this does not look safe at all.

Maybe we should instead swap the cleanup order in ShenandoahHeap::op_cleanup_traversal(): first
recycle all the trash, and *then* reset the bitmaps. Then we don't have to test for
region->is_trash() in ShResetNextBitmapTraversal at all (we should still assert it), resolving the
original race.

Thanks,
-Aleksey