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

[Stefan Karlsson]

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

   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?


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



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?



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?

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.

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