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

[coleen.phillimore at oracle.com]

On 11/12/18 9:31 AM, Thomas Schatzl wrote:
> Hi Coleen,
>
>    thanks for your review.
>
> On Mon, 2018-11-12 at 09:15 -0500, coleen.phillimore at oracle.com wrote:
>> Thomas, thank you so much for cleaning this up.
>>
>>
> http://cr.openjdk.java.net/~tschatzl/8213307/webrev.0_to_1/test/hotspot/jtreg/runtime/MemberName/MemberNameLeak.java.html
>> I think this test should have:
>>    * @requires vm.opt.final.ClassUnloading
> The test executes its own VM via processtools, and we make sure that
> that one has class unloading enabled. So I do not think the test itself
> requires an @requires.
>
>> The RMT cleanup (and now triggering) was put in
>> SystemDictionary::do_unloading as a convenience rather than spread
>> the call in all 4/5 GCs. It's not really related to class unloading
>> like the other cleanups.   If there's a better place for it, we can
>> move it.
>>
>> Also now that the timers are only timing triggering cleanup, it's
>> not really interesting to have them for all the actions.  I think
>> there should just be one timer around the
>> whole SystemDictionary::do_unloading
>> and not pass the flag.  I'll take your follow on RFE.
> Yes, I will follow up with an RFE (I need to check if I already filed
> one).
>
>> This change looks good apart from the comment above, which I don't
>> need to see a webrev if you agree.
> Unfortunately, not ;) Can you look over the test again and agree with
> me that the VM where we actually test the RMT cleanup is guaranteed to
> run with class unloading enabled so that the code is executed?

I see it only runs with GCs that support class unloading, but it still 
seems kind of pointless to run the the test where the driver doesn't 
support class unloading.   It's fine as is though.  Thank you for doing 
the cleanup.

Thanks,
Coleen
>
> Thanks,
>    Thomas
>
>