RFR (M) 8213753: SymbolTable is double walked during class unloading and clean up table timing in do_unloading

[coleen.phillimore at oracle.com]

I added the timing for triggering cleanup.  It's likely not 
interesting.  The main cleanup time is code cache cleaning still.

This is my fastdebug timing:
[4.992s][debug][gc,phases] GC(2) ClassLoaderData 0.016ms
[4.992s][debug][gc,phases] GC(2) Trigger cleanups 0.010ms
[5.039s][debug][gc,phases] GC(2) Class Unloading 47.290ms

open webrev at http://cr.openjdk.java.net/~coleenp/2019/8213753.02/webrev

Retested with tier1, 2, kitchensink sanity, and specjbb2015.

Thanks for the code reviews.

Thanks,
Coleen

On 1/30/19 9:01 AM, Aleksey Shipilev wrote:
> On 1/30/19 2:27 PM, coleen.phillimore at oracle.com wrote:
>>> *) In SystemDictionary::do_unloading, do we think that trigger_cleanup() are cheap? Otherwise it
>>> makes sense to retain a single GCTraceTime block around all three trigger_cleanups?
>> The three trigger cleanups aren't specifically ClassLoaderData, so I didn't include them in the
>> timing.  I think people/tools might look for ClassLoaderData so I didn't want to change the name of
>> the timing.   Checking now...
>>
>>      GCTraceTime(Debug, gc, phases) t("ClassLoaderData", gc_timer);
>>
>> I thought of removing it completely but the timing but the enclosing timers (exception in
>> shenandoah) also include timing CodeCache::do_unloading and clean_weak_klass_links which are more
>> expensive than ClassLoaderData.
> My only (little) concern was that SD::do_unloading now has only one GCTraceTime("ClassLoaderData").
> Without knowing beforehand if ::trigger_cleanup-s are cheap, it seems odd to drop GCTraceTime from
> them. Something like:
>
>    GCTraceTime(Debug, gc, phases) t("Trigger cleanups", gc_timer);
>
>    ResolvedMethodTable::trigger_cleanup();
>
>    if (unloading_occurred) {
>       SymbolTable::trigger_cleanup();
>
>       // Oops referenced by the protection domain cache table may get unreachable independently
>       // of the class loader (eg. cached protection domain oops). So we need to
>       // explicitly unlink them here.
>       // All protection domain oops are linked to the caller class, so if nothing
>       // unloads, this is not needed.
>       _pd_cache_table->trigger_cleanup();
>    }
>
> ...but that can be indeed skipped if we know that those triggers cost much less than the timer itself.
>
> -Aleksey
>