RFR: 8056240: Investigate increased GC remark time after class unloading changes in CRM Fuse

[Stefan Karlsson]

On 2014-10-14 14:41, Mikael Gerdin wrote:
> Stefan,
>
> On 2014-10-13 15:11, Stefan Karlsson wrote:
>> Hi again,
>>
>> Latest changes after comments from Bertrand and Coleen:
>> http://cr.openjdk.java.net/~stefank/8056240/webrev.01
>
> chunkedList.hpp
> 902 +    // Don't clear the next pointers since that would interfer
> Typo: interfer -> interfere

Done.

>
> thread.hpp
> Can't you forward-declare MetadataOnStackBuffer instead of #including it?
> You only do simple manipulation on the pointers so it should work.

Done.

I tried to do it earlier but couldn't get it to work. The following 
seems to work:
class Metadata;
template <class T, MEMFLAGS F> class ChunkedList;
typedef ChunkedList<Metadata*, mtInternal> MetadataOnStackBuffer;

>
> Ideally, it would be nice to not have the MetadataOnStackBuffer* in 
> Thread but in one of its subclasses. Unfortunately it's used by both 
> WorkerThread and VMThread so I don't have a good solution for that. I 
> just find the pollution of the Thread superclass somewhat unfortunate.

I had the same thoughts.

>
> classLoaderData.hpp
> Why is clean_metaspaces public?

I moved it to the private section.

>
> nmethod.cpp
> 381 +  // In this lmetadata, we must only follow those metadatas 
> directly embedded in
> Typo: lmetadata -> metadata

Done.

>
> 401 +    // Call function Method*, not embedded in these other places.
> In the above comment you talk about "visiting metadata", can you 
> change these comments to use consistent terminology?

Done. I also fixed the place where I copied this comment from.

>
> Otherwise the change looks ok.

thanks,
StefanK

>
> /Mikael
>
>
>> http://cr.openjdk.java.net/~stefank/8056240/webrev.01.delta
>>
>> 1) Updated AccessFlags:set_on_stack to use a looping cmpxchg
>> implementation.
>> 2) Remove MetadataOnStackMark::_doit and restructure
>> ClassLoaderDataGraph::do_unloading instead.
>> 3) Move MetadataOnStackBuffer and make it a utility class named
>> ChunkedList.
>>
>> thanks,
>> StefanK
>>
>> On 2014-10-02 12:53, Stefan Karlsson wrote:
>>> Hi all,
>>>
>>> (The following patch changes HotSpot code in areas concerning GC, RT,
>>> and Compiler. So, it would be good to get reviews from all three 
>>> teams.)
>>>
>>> Please, review this patch to optimize and parallelize the CodeCache
>>> part of MetadaOnStackMark.
>>>
>>> G1 performance measurements showed longer than expected remark times
>>> on an application using a lot of nmethods and Metadata. The cause for
>>> this performance regression was the call to
>>> CodeCache::alive_nmethods_do(nmethod::mark_on_stack); in
>>> MetadataOnStackMark. This code path is only taken when class
>>> redefinition is used. Class redefinition is typically used in
>>> monitoring and diagnostic frameworks.
>>>
>>> With this patch we now:
>>> 1) Filter out duplicate Metadata* entries instead of storing a
>>> Metadata* per visited metadata reference.
>>> 2) Collect the set of Metadata* entries in parallel. The code
>>> piggy-backs on the parallel CodeCache cleaning task.
>>>
>>> http://cr.openjdk.java.net/~stefank/8056240/webrev.00/
>>> https://bugs.openjdk.java.net/browse/JDK-8056240
>>>
>>> Functional testing:
>>> JPRT, Kitchensink, parallel_class_unloading, unit tests
>>>
>>> Performance testing:
>>> CRM Fuse - where the regression was found
>>>
>>> The patch changes HotSpot code in areas concerning GC, RT, Compiler,
>>> and Serviceability. It would be good to get some reviews from the
>>> other teams, and not only from the GC team.
>>>
>>> thanks,
>>> StefanK
>>