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