RFR: Remove ShenandoahMarkCompactBarrierSet

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

*) 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.

*) 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;
   }
 }

*) 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.

Otherwise good.

Thanks,
-Aleksey