RFR (L) 8061205: MetadataOnStackMark only needs to walk code cache during class redefinition

[Coleen Phillimore]

Stefan,

I made the changes you suggested.  I also moved purge_previous_versions 
conditionally back to class unloading, where G1 doesn't do this until 
full GC.   Do you prefer this to calling the separate function for 
ClassLoaderDataGraph::clean_redefinition_metadata()?  I thought with a 
separate function we could be more judicious where to call it, but we 
can also add conditions to ClassLoaderDataGraph::do_unloading() to avoid 
metadata walking.

http://javaweb.us.oracle.com/~cphillim/webrev/8061205.03/

I've rerun all the class redefinition tests with -XX:+UseG1GC and with 
the default collector.

Thanks,
Coleen

On 3/10/15, 3:50 PM, Coleen Phillimore wrote:
>
> Stefan,   Thank you for reviewing this so quickly!
>
> On 3/10/15, 12:24 PM, Stefan Karlsson wrote:
>> Hi Coleen,
>>
>> Thanks for fixing this and lowering the G1 remark times when class 
>> redefinition is used.
>>
>> I'm only reviewing the GC specific parts:
>>
>> On 2015-03-09 21:57, Coleen Phillimore wrote:
>>> Summary: Only do full metadata walk during class redefinition and 
>>> only walk handles during class unloading.
>>>
>>> This change decouples metadata walking for redefinition and class 
>>> unloading, so that class unloading for G1 doesn't walk the code 
>>> cache.  It also decouples GC and on_stack marking in the code cache.
>>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/8061205/
>>
>>
>> http://cr.openjdk.java.net/~coleenp/8061205/src/share/vm/classfile/metadataOnStackMark.cpp.frames.html 
>>
>>
>> There are Atomic calls left to handle concurrent retiring of buffers. 
>> Do you want to keep it?:
>
> I didn't know whether I should or not.  I suppose we should trust our 
> source code control system in case we ever need to do this 
> concurrently again.  I'll remove the concurrency - it makes the code a 
> bit simpler.   The code to do chunked lists is still good though, so 
> I'm glad you added that as a utility class.
>>
>>   97 void MetadataOnStackMark::retire_buffer(MetadataOnStackBuffer* 
>> buffer) {
>>   98   if (buffer == NULL) {
>>   99     return;
>>  100   }
>>  101
>>  102   MetadataOnStackBuffer* old_head;
>>  103
>>  104   do {
>>  105     old_head = const_cast<MetadataOnStackBuffer*>(_used_buffers);
>>  106     buffer->set_next_used(old_head);
>>  107   } while (Atomic::cmpxchg_ptr(buffer, &_used_buffers, old_head) 
>> != old_head);
>>  108 }
>>
>> There's also some Atomic code in accessFlags that were added to 
>> support concurrent mark_on_stack calls. Maybe you want to get rid of 
>> that code as well?
>
> I like how you changed it so that the cpool and method is only 
> recorded if it's not already marked.  That probably saves a lot of 
> time and space.
>>
>>
>> http://cr.openjdk.java.net/~coleenp/8061205/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp.cdiff.html 
>>
>>
>> Could you get rid of the pre/post_work_verification functions? They 
>> were only added to be able to verify the MetadataOnStackMark state, 
>> and is not needed anymore.
>>
>>     void pre_work_verification() {
>> - 
>> assert(!MetadataOnStackMark::has_buffer_for_thread(Thread::current()), "Should 
>> be empty");
>>     }
>>       void post_work_verification() {
>> - 
>> assert(!MetadataOnStackMark::has_buffer_for_thread(Thread::current()), "Should 
>> be empty");
>>     }
>>
>>
>
> Okay, thanks.  I didn't know if you'd prefer that.
>
>>
>> http://cr.openjdk.java.net/~coleenp/8061205/src/share/vm/runtime/thread.cpp.frames.html 
>>
>>
>> 4105 void Threads::metadata_handles_do(void f(Metadata*)) {
>> 4106   // Only walk the Handles in Thread.
>> 4107   ALL_JAVA_THREADS(p) {
>> 4108     p->metadata_handles_do(f);
>> 4109   }
>> 4110 }
>>
>> This only walks metadata handles in the Java threads. Don't we have 
>> metadata handles in the VM  Thread?
>>
>>
>
> Hm.   We never walked non-Java threads before but yes, we could have 
> Metadata handles on these threads.  Wow, thanks for finding this 
> bug!   I think this should walk GC threads too (no?)
>
>>
>> http://cr.openjdk.java.net/~coleenp/8061205/src/share/vm/gc_implementation/shared/vmGCOperations.cpp.frames.html 
>>
>>
>> Could you motivate why only Metadata induced Full GCs need to do this 
>> unloading?
>>
>>  268   // If redefinition, make a pass over the metadata to find any 
>> that
>>  269   // can be marked to be deallocated
>>  270   if (JvmtiExport::has_redefined_a_class()) {
>>  271     ClassLoaderDataGraph::clean_redefinition_metadata();
>>  272   }
>>
>> Don't we need it for our other Full GCs?
>
> No, I don't think we want it for all full GCs.   The effect of 
> cleaning out the redefinition metadata is to remove the metadata 
> itself.  There may be some mirrors unused from the redefined classes.  
> I suppose you could construct a test case where one of the mirrors is 
> gigantic and causes an out of memory situation in Java heap.
>
> The main effect of walking the previous versions is to find more 
> metadata to clean out though, for the next attempt at class unloading.
>>
>> I would prefer if this code could be kept inside the GC code or where 
>> it used to be, inside the ClassLoaderDataGraph::do_unloading 
>> function. With the current change, the 
>> VM_CollectForMetadataAllocation class is burdened with the internal 
>> knowledge about class redefinition and class unloading.
>
> In ClassLoaderDataGraph::do_unloading() is used when you're not in a 
> Full GC, which is the problem.  It can't go there unless we pass down 
> the logic that we're in a full GC or not.  I think this is messier.  I 
> think this VM_CollectForMetadataAllocation seems the right place to 
> clean up metadata, if needed.   One place or another, there has to be 
> knowledge of class redefinition.
>
> Actually, my original change had it not called at all, since this code 
> is executed for every class redefinition.  Then I thought something 
> should call it...   The bias should be to avoid calling this function 
> though because generally it doesn't find very much to do.
>
> I thought a better place to call this would be for last-ditch 
> collections, but I didn't know exactly where that was.
>
> Coleen
>>
>> Thanks,
>> StefanK
>>
>>> bug link https://bugs.openjdk.java.net/browse/JDK-8061205
>>>
>>> Tested with FMW performance runs. vm.quick.testlist, 
>>> jdk/test/java/lang/instrument tests and JPRT.
>>>
>>> Thanks,
>>> Coleen
>>
>