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

[Mikael Gerdin]

Stefan,

On 2014-10-14 15:02, Stefan Karlsson wrote:
>
> 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;

I still prefer this variant even though it's not particularly pretty.

>
>>
>> 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,

Thanks for fixing this Stefan.

/Mikael

> 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
>>>
>