RFR: JDK-8226757: Shenandoah: Make Traversal a separate mode

Roman Kennke rkennke at redhat.com
Wed Jul 3 16:16:34 UTC 2019


Hi Aleksey,

Thanks for reviewing!

>> There was a small mistake in one of the tests. Updated webrev:
>>
>> http://cr.openjdk.java.net/~rkennke/JDK-8226757/webrev.02/
> *) Looks like the synopsis is incorrect. Should be e.g. "Make passive/normal/traversal modes
> explicit", or something.

Changed it to: "Shenandoah: Make traversal and passive modes explicit"

> *) Please make sure it builds with --disable-precompiled-headers

Done. Required logging includes.

> *) Changes like these are not exactly for making a separate mode? The condition seems superficial
> even without explicit mode: ShenandoahSATBBarrier should not be active with traversal? If so, might
> be worthwhile to split out, this would make current changeset a bit less denser.
> 
> -    if (ShenandoahSATBBarrier && !dest_uninitialized &&
> !ShenandoahHeap::heap()->heuristics()->can_do_traversal_gc()) {
> +    if (ShenandoahSATBBarrier && !dest_uninitialized) {

Split out and pushed under:
https://bugs.openjdk.java.net/browse/JDK-8227199

> *) Rewrite this:
> 
>   GCMode default_mode = heap->is_traversal_mode() ? concurrent_traversal : concurrent_normal;
>   GCCause::Cause default_cause = default_mode == concurrent_traversal ?
> GCCause::_shenandoah_traversal_gc : GCCause::_shenandoah_concurrent_gc;
> 
> Into this:
> 
>   GCMode default_mode = heap->is_traversal_mode() ?
>                            concurrent_traversal : concurrent_normal;
>   GCCause::Cause default_cause = heap->is_traversal_mode() ?
>                            GCCause::_shenandoah_traversal_gc : GCCause::_shenandoah_concurrent_gc;

Done.

> *) shenandoah_globals.hpp still mention "passive" is heuristics, not mode. Happens in both
> ShenandoahGCHeuristics and ShenandoahGCMode option descriptions.

Fixed.

> *) TestObjItrWithHeapDump.java, TestClassLoaderLeak.java: indenting is confusing. Suggestion:
> 
>      String[][][] modeHeuristics = new String[][][] {
>           {{"normal"},    {"adaptive", "compact", "static", "aggressive"}},
>           {{"traversal"}, {"adaptive"}},
>           {{"passive"},   {"passive"}}
>      };

Done.

>  ...by the way, shouldn't "traversal" mode have "traversal" heuristics here? I see
> shenandoahTraversalMode equate "adaptive" to ShenandoahTraversalHeuristics, which I believe is
> confusing...

Well, the default traversal heuristics mimics the normal adaptive
heuristics as much as possible, and it is also adaptive. IMO it makes
sense to call it adaptive heuristics. I am about to also add an
aggressive heuristic to traversal mode. I'd rather keep it.

> *) @run lines indenting is not correct in TestRefprocSanity.java

Fixed.

Incremental:
http://cr.openjdk.java.net/~rkennke/JDK-8226757/webrev.03.diff/
Full:
http://cr.openjdk.java.net/~rkennke/JDK-8226757/webrev.03/

Good?

Roman




More information about the hotspot-gc-dev mailing list