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

[Stefan Karlsson]

On 2014-10-13 18:09, Bertrand Delsart wrote:
> Thanks Stefan,
>
> Code looks cleaner and safer from a runtime point of view.
>
> Just a few minor points (feel free to ignore them) and a few typos.
>
> First, note that there is no loop in ConstantPool::set_on_stack when 
> the CAS fails but this seems OK. This should not lead to any 
> regression since the previous version was not thread safe. Hence, this 
> works if you are sure that no other change that setting _on_stack can 
> happen concurrently in your new use case. To be safer, I would add an 
> assert:
>       int result = Atomic::cmpxchg(new_flags, &_flags, old_flags);
>
>       if (result == old_flags) {
>         // Succeeded.
>         MetadataOnStackMark::record(this, Thread::current());
>       } else {
> +        assert((result & _on_stack) != 0, "Expected only on_stack to 
> be set concurrently");
>       }

Done. I added a loop.

>
> Another minor point: in classLoaderData.cpp, your new 
> free_deallocate_list loop at the end of 
> ClassLoaderDataGraph::clean_metaspaces could be replaced by a call to 
> your new ClassLoaderDataGraph::free_deallocate_lists(), which performs 
> exactly the same work.

Done.

>
> Last minor point: the retire_buffer_for_thread(Thread::current()) at 
> the end of work_first_pass looks sufficient. However, to stress your 
> change and improve the robustness against future changes, should we 
> assert that the buffer for the current thread is NULL at the end of 
> G1ParallelCleaningTask::work ? This assert would detect any late 
> marking (which would be useless and might also disturb the next GC 
> cycle). If we are paranoid, we can also add the same assert at the 
> beginning of G1ParallelCleaningTask::work. I'll let you see whether 
> this is worth it since this impacts only GC code.

Done.

>
> For similar reason, I was wondering whether we should assert that 
> _used_buffers is  NULL at the beginning of MetadataOnStackMark 
> constructor. This would allow to detect any marking done out of a 
> MetadataOnStackBuffer section. Such marking would look suspicious to 
> me. If you have any doubt, we can let Coleen add the assert later.

Done.
>
> Except for that, there are a few typos in the comments. Will let a 
> native English speaker make an additional pass over them. Here are the 
> ones that stood out when I reviewed the code changes:
>
> concurrentMark.cpp:
> +  // In this lmetadata, we must only follow those metadatas directly 
> embedded in

This wasn't written by me. It comes from the comment in 
nmethod::metadata_do. :) I've removed the l from lmetdata, but I haven't 
change the wording of that comment.

>
> g1CollectedHeap.cpp
> +    // The nmethod cleaning helps out an does the CodeCache part of 
> MetadataOnStackMark.

I changed an => and.

I'll send out a new webrev after I've taken care of Mikael's comments.

thanks,
StefanK

>
> Regards,
>
> Bertrand (not a Reviewer).
>
>
> On 13/10/14 15:11, Stefan Karlsson wrote:
>> Hi again,
>>
>> Latest changes after comments from Bertrand and Coleen:
>> http://cr.openjdk.java.net/~stefank/8056240/webrev.01
>> 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
>>
>
>