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

[Bertrand Delsart]

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");
       }

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.

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.

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.

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

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

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
>


-- 
Bertrand Delsart,                     Grenoble Engineering Center
Oracle,         180 av. de l'Europe,          ZIRST de Montbonnot
38334 Saint Ismier,                                        FRANCE
bertrand.delsart at oracle.com             Phone : +33 4 76 18 81 23

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
NOTICE: This email message is for the sole use of the intended
recipient(s) and may contain confidential and privileged
information. Any unauthorized review, use, disclosure or
distribution is prohibited. If you are not the intended recipient,
please contact the sender by reply email and destroy all copies of
the original message.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~