RFR: 8242054: Shenandoah: New incremental-update mode

Roman Kennke rkennke at redhat.com
Sat Apr 4 00:26:31 UTC 2020


>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8242054
>> Webrev:
>> http://cr.openjdk.java.net/~rkennke/JDK-8242054/webrev.00/

I rebased this patch on top of "JDK-8242130: Shenandoah: Simplify
arraycopy-barrier dispatching"

Now that we purged traversal-mode, I also added tests for IU mode, by
(mostly) mechanically adding back the tests that I removed with
Traversal mode, and change ShenandoahGCMode=traversal ->
ShenandoahGCMode=iu.
 	
> *) 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) ...

Right. Fixed the occurances that I found (the mentioned one is gone with
JDK-8242130.

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

Ok. I turned it into:
SHENANDOAH_CHECK_FLAG_SET(ShenandoahSATBBarrier ||
ShenandoahStoreValEnqueueBarrier)

which required the macro to do !(name) instead of !name.

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

This should be gone with JDK-8242130, plus in this revision I also made
the clone_barrier() to dispatch in a very similar fashion.

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

Right. Good catch!

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

Because Threads::oops_do() allocates some Resource stuff. It's
everywhere where Thread::oops_do() is called. Should probably be
looked-at separately?


http://cr.openjdk.java.net/~rkennke/JDK-8242054/webrev.02/

Better?

Roman



More information about the shenandoah-dev mailing list