RFR: 8242054: Shenandoah: New incremental-update mode

[Aleksey Shipilev]

On 4/2/20 9:23 PM, Roman Kennke wrote:
> Some of our users like some properties of the marking/traversal
> behaviour of the Traversal mode, but would also like the benefits and
> ergonomics of the normal mode. It turns out that it is relatively easy
> to transplant the barrier-model of Traversal mode into the regular mode.
> This is essentially a variant of the well-known incremental-update model
> of concurrent marking.
> 
> We already have all the barrier-foo that we need from the Traversal
> mode. The following modifications are required to make it work with
> regular mode:
> 
> - Arraycopy-barriers must mark through src instead of dst (we're
> interested in the new-values not in the previous-valuses). They must
> also trigger when dest is uninitialized (esp in the ASM prologue calls)
> - Related to this, clone-barriers now also need to mark through the
> src-object. (SATB doesn't need this because destination is by definition
> uninitialized).
> - Both clone and arraycopy-barriers can skip marking the src obj/array
> if the src is allocated-after-mark-start: in this case it's guaranteed
> by the barrier model than any reference in them must already have gone
> through a barrier.
> - C2 code sees some adjustments to allow marking-active checks when
> expanding ShenandoahStoreValEnqueueBarrierNode. It previously only
> checked HAS_FORWARDED to account for Traversal-active. It also adds
> MARKING to the state-check for the clone-barrier.
> - I removed the flags-checks from aggressive/compact/static heuristics.
> This is now checked in shenandoahNormalMode.cpp (and is consistent with
> adaptive)
> - I introduced a new mode called ShenandoahIUMode, mapped to
> ShenandoahGCMode=iu, which sets the flags for this new mode.
> - I added no testing yet. The plan is to change traversal tests to iu
> tests if/when Traversal gets eliminated.
> - I did, however, test everything in shenandoah/jdk by changing the
> default mode to iu.
> 
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8242054
> Webrev:
> http://cr.openjdk.java.net/~rkennke/JDK-8242054/webrev.00/

*) I would prefer for flag checks to be first. It makes it clearer to me that path is only taken in
some configurations. Example, this:
  if (dest_uninitialized && ShenandoahSATBBarrier) ...
 ...becomes this:
  if (ShenandoahSATBBarrier && dest_uninitialized) ...

*) These blocks should not be removed. At very least we want to make the check for SATB/StoreVal
barrier conditional on the mode. But other barrier checks should definitely stay.

  47   // Final configuration checks
  48   SHENANDOAH_CHECK_FLAG_SET(ShenandoahLoadRefBarrier);
  49   SHENANDOAH_CHECK_FLAG_SET(ShenandoahSATBBarrier);
  50   SHENANDOAH_CHECK_FLAG_SET(ShenandoahCASBarrier);
  51   SHENANDOAH_CHECK_FLAG_SET(ShenandoahCloneBarrier);

*) This is some local madness:

  84   int gc_state = _heap->gc_state();
  85   if ((gc_state & ShenandoahHeap::MARKING) != 0 &&
  86       ShenandoahStoreValEnqueueBarrier &&
  87       !_heap->marking_context()->allocated_after_mark_start(obj)) {
  88     ShenandoahUpdateRefsForOopClosure<false, false, true> cl;
  89     obj->oop_iterate(&cl);
  90   } else if ((gc_state & ShenandoahHeap::HAS_FORWARDED) != 0) {
  91     if (skip_bulk_update(cast_from_oop<HeapWord *>(obj))) return;

Do we really need to read the gc_state and then mask it, instead of just checking
heap->is_concurrent_mark_in_progress()? Why check for has_forwarded on the else branch? asserting
has_forwarded_objects is futile on that branch too.

It looks to me the whole thing is:

  if (ShenandoahStoreValEnqueueBarrier && _heap->is_concurrent_marking_in_progress() &&
       !_heap->marking_context()->allocated_after_mark_start(obj)) {
     ShenandoahUpdateRefsForOopClosure<false, false, true> cl;
     obj->oop_iterate(&cl);
     return;
   }

   assert(_heap->has_forwarded_objects(), "only when heap is unstable");
   if (skip_bulk_update(cast_from_oop<HeapWord *>(obj))) return;

   if (_heap->is_evacuation_in_progress()) {
     ...
   } else if (_heap->is_concurrent_traversal_in_progress()) {
     ...
   } else {
     ...
   }

*) I don't think the rewrite in ShFinalMarkTask is equivalent. See, on the path here:

 262 ShenandoahSATBAndRemarkCodeRootsThreadsClosure tc(&cl,
 263                                     ShenandoahStoreValEnqueueBarrier ? &mark_cl : NULL,
 264                                     &blobsCl);

 ...we used to pass NULL as blobsCL when do_nmethods is false.

*) Also, why ResourceMark here?

 199         if (_cl != NULL) {
 200           ResourceMark rm;
 201           thread->oops_do(_cl, _code_cl);

-- 
Thanks,
-Aleksey