RFR: 8242054: Shenandoah: New incremental-update mode

Roman Kennke rkennke at redhat.com
Mon Apr 6 13:12:41 UTC 2020


>> I made a mistake with webrev.02, it included the change for JDK-8242130.
>> Please review this webrev instead:
>>
>> http://cr.openjdk.java.net/~rkennke/JDK-8242054/webrev.03/
> 
> *) shenandoahBarrierSetAssembler_aarch64.cpp: flag check should be first?
> 
>   62       if (dest_uninitialized && !ShenandoahStoreValEnqueueBarrier) {

Yeah. In-fact, it is cleaner to check for ShenandoahSATBBarrier instead.

> *) I wonder if there is a test that checks ShenandoahSATBBarrier and
> ShenandoahStoreValEnqueueBarrier are not enabled at the same time. If not, I can add it later.

I don't think we have this.

> *) I wonder ShenandoahBarrierSet::clone_barrier should actually check for ShenandoahHeap::UPDATEREFS
> on L110, like ShenandoahBarrierSet::arraycopy_barrier started doing it recently?
>   http://hg.openjdk.java.net/jdk/jdk/rev/0c8345a2ad50#l4.36
> 
>  105   int gc_state = _heap->gc_state();
>  106   if ((gc_state & ShenandoahHeap::MARKING) != 0) {
>  107     clone_marking(obj);
>  108   } else if ((gc_state & ShenandoahHeap::EVACUATION) != 0) {
>  109     clone_evacuation(obj);
>  110   } else {
>  111     clone_update(obj);
>  112   }

I it is not necessary. All entries check that we only get there when we
have HAS_FORWARDED set. We also assert that update-refs is in progress
in clone_update(). In arraycopy it is slightly different because the
entry that comes from runtime-arraycopy does not check HAS_FORWARDED.

> *) In ShenandoahSATBAndRemarkCodeRootsThreadsClosure, double space in "else  if" here:
>  202         } else  if (_code_cl != NULL) {

Oops. Fixed.

> *) In TestObjItrWithHeapDump.java, the indent is wrong:
>   60              {{"normal"},    {"adaptive", "compact", "static", "aggressive"}},
>   61              {{"iu"}, {"adaptive", "aggressive"}},
>   62              {{"passive"},   {"passive"}}

Fixed.

> *) Same thing in TestClassLoaderLeak.java:
> 
>  128         String[][][] modeHeuristics = new String[][][] {
>  129              {{"normal"},    {"adaptive", "compact", "static", "aggressive"}},
>  130              {{"iu"}, {"adaptive", "aggressive"}},
>  131              {{"passive"},   {"passive"}}
>  132         };

Fixed.

> *) In TestWrongArrayMember.java, the indent is wrong:
> 
>   30  * @run main/othervm -Xmx128m -XX:+UnlockExperimentalVMOptions -XX:+UseShenandoahGC
>                     TestWrongArrayMember
>   31  * @run main/othervm -Xmx128m -XX:+UnlockExperimentalVMOptions -XX:+UseShenandoahGC
> -XX:ShenandoahGCMode=iu TestWrongArrayMember

Fixed.

> *) TestHeuristicsUnlock.java change actually raises a question: should this mode be experimental?
>   52         testWith("-XX:ShenandoahGCMode=iu", Mode.PRODUCT);

I am not sure. On one side we now have all the tests that we considered
sufficient for traversal in place. Should we make it experimental, run
it for a while in our CIs (which also needs changing from traversal ->
iu), and then declare it product soon-ish? What do you think?

Above fixes, still product-flag:
http://cr.openjdk.java.net/~rkennke/JDK-8242054/webrev.04/

Roman




More information about the hotspot-gc-dev mailing list