RFR (S): 8213307: G1 should clean up RMT with ClassUnloadingWithConcurrentMark

Thomas Schatzl thomas.schatzl at oracle.com
Mon Nov 5 14:28:24 UTC 2018


Hi,

On Mon, 2018-11-05 at 15:11 +0100, Aleksey Shipilev wrote:
> On 11/05/2018 01:53 PM, Thomas Schatzl wrote:
> > I also made the test to not go into an infinite loop if no RMT
> > unloading occurs as this would make it wait on external timeout
> > which seems ugly. If somebody disagrees with that change I can
> > easily revert it.
> 
> I'd keep the original behavior and wait for external timeout. Reason:
> I am not sure 5 iterations is enough for every GC :)

I have no particular opinion about this, so I changed this particular
hunk back. Note that all current Hotspot GCs, if either
ClassUnloadingWithConcurrentMark/ClassUnloading is enabled, do class
unloading at every gc.

If e.g. Shenandoah would not do that by default, that might be a
significant and potentially surprising difference in behavior compared
to other GCs.

> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8213307
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8213307/webrev/
> 
> Looks okay to me.
> 
> *) While we are at it, both these GCTraceTime hooks seem to be
> misplaced. They should be moved where the actual cleanup is taking
> place:
> 
> 1877   {
> 1878     GCTraceTime(Debug, gc, phases)
> t("ProtectionDomainCacheTable", gc_timer);
> 1879     // Oops referenced by the protection domain cache table may
> get unreachable independently
> 1880     // of the class loader (eg. cached protection domain oops).
> So we need to
> 1881     // explicitly unlink them here.
> 1882     _pd_cache_table->trigger_cleanup();
> 1883   }
> 1884
> 1885   GCTraceTime(Debug, gc, phases) t("ResolvedMethodTable",
> gc_timer);
> 1886   ResolvedMethodTable::trigger_cleanup();
> 1887
> 
> That should probably go to another RFR. Still, it seems cleaner to
> have the scope around RMT::trigger_cleanup to make sure the
> GCTraceTime relates to that operation only, in case some new
> code appears after it.

I can agree with adding the scope for this change. I will file a new CR
to move them to the actual cleanup. 

> *) I'd consider conditionalizing ClassUnloadingWithConcurrentMark too
> in the test -- we want to cover the Full GC case too, no?

I am not sure how ClassUnloadingWithConcurrentMark affects class
unloading during full gc. I will add -XX:+ClassUnloading to the test
though to make sure that class unloading is done always regardless of
gc type in the VM that is started (although both are on by default).

I.e. -XX:+ClassUnloading -XX:-ClassUnloadingWithConcurrentMark should
unload classes during full gc only (whatever "full gc" means).

New webrevs:
http://cr.openjdk.java.net/~tschatzl/8213307/webrev.0_to_1/ (diff)
http://cr.openjdk.java.net/~tschatzl/8213307/webrev.1/ (full)

Thanks,
  Thomas




More information about the hotspot-gc-dev mailing list