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 shenandoah-dev
mailing list