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

[Coleen Phillimore]

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
>