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

[Erik Österlund]

Hi Aleksey,

On 2018-11-05 15:49, Aleksey Shipilev wrote:
> On 11/05/2018 03:28 PM, Thomas Schatzl wrote:
>> 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.
> Shenandoah does class unloading every N-th concurrent cycle, and default N is coincidentally 5,
> which is on the suggested threshold for this test. We have this test hooked up for Shenandoah, and
> would like to keep it passing.
>
> Talking about surprises in current Hotspot GCs: ZGC does not even do class unloading, even if users
> "forces" it with -XX:+ClassUnloading(WithConcurrentMark) -- which is probably why we don't see ZGC
> config in that test.

We have not implemented STW class unloading in ZGC, because we did not 
think it is appropriate for a concurrent collector to do that in a 
safepoint. We are going straight for concurrent class unloading instead, 
which is currently in the process of integrating. The infrastructure 
being built to support this is rather generic, and ultimately you will 
probably be able to hook both G1 and Shenandoah into it if you like, on 
platforms supporting nmethod entry barriers.

Thanks,
/Erik

>>> *) 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).
> Yeah, I was first thinking to test the path where +ExplicitGCInvokesConcurrent, but
> -XX:-ClassUnloadingWithConcurrentMark. But then the test would fail, because explicit System.gc()
> from the test would not clean up RMT, and that would arguably be the intended behavior. So this
> config would be irrelevant.
>
> Adding -XX:+ClassUnloading is a good touch.
>
>> New webrevs:
>> http://cr.openjdk.java.net/~tschatzl/8213307/webrev.0_to_1/ (diff)
>> http://cr.openjdk.java.net/~tschatzl/8213307/webrev.1/ (full)
> Looks good to me!
>
> -Aleksey
>