RFR: Remove ShenandoahMarkCompactBarrierSet

[Roman Kennke]

Am 23.04.2018 um 15:47 schrieb Aleksey Shipilev:
> On 04/23/2018 02:10 PM, Roman Kennke wrote:
>> This came out of the upstream merge that I'm working on. Upstream
>> doesn't allow to re-set the BarrierSet and I think this is good.
>> However, we are temporarily re-setting the BS during mark-compact to
>> prevent accidentally calling into RB and resolving garbage (because
>> fwd-ptr is not always pointing to valid object during adjust-ptrs phase).
>>
>> I am proposing to revert the hack and remove ShMCBS and instead check
>> has_forwarded_objects() in the RB. And of course, un-setting
>> has_forwarded_objects after mark-compact-phase1-marking. I would argue
>> that this is reasonable because the fwd ptr is not guaranteed to point
>> to anything meaningful anyway.
>>
>> http://cr.openjdk.java.net/~rkennke/mark-compact-bs/webrev.00/
> 
> Makes sense. So, setting BS is now the error in upstream jdk/jdk?

Yes. Except for the initialization.

> *) I thought we are not dropping the assert in ShenandoahConcurrentMark::mark_through_ref? If we do,
> there is a symmetrical assert when polling the object from the task-queue, see
> ShenandoahConcurrentMark::do_task. If the removed assert fails, then that leftover assert should
> fail too.

Oops, that was a leftover mistake. Removed.

> *) I think we should put the comment in SBS::read_barrier about Full GC path. Otherwise it looks
> like a proactive performance optimization, and not the correctness fix. Suggestion:
> 
>  oop ShenandoahBarrierSet::read_barrier(oop src) {
>    // Check for forwarded objects, because on Full GC path we might deal with
>    // non-trivial fwdptrs that contain Full GC specific metadata. We could check
>    // for is_full_gc_in_progress(), but this also covers the case of stable heap,
>    // which provides a bit of performance improvement.
>    if (ShenandoahReadBarrier && _heap->has_forwarded_objects()) {
>      return ShenandoahBarrierSet::resolve_forwarded(src);
>    } else {
>      return src;
>    }
>  }

Thanks. Done.

> *) Grammar: "We don't necessarily need to update refs..."
> 
> 182       // TODO: We don't strictly always need to update refs. We might want to clean
> 183       // up managing has_forwarded_objects when diving into degen/full-gc.

Also fixed.

http://cr.openjdk.java.net/~rkennke/mark-compact-bs/webrev.01/

Good to go now?

Roman