Request for review 8007320: NPG: move method annotations

[serguei.spitsyn at oracle.com]

Coleen,


Sorry for some latency, but your fixes are non-trivial.
I have to say: "Wow, it is a lot of work!"


agent/src/share/classes/sun/jvm/hotspot/oops/ConstMethod.java

   435   return (getSize() * wordSize) - (offset * wordSize) - 2;

  Nit:

   435   return wordSize * (getSize() - offset) - sizeof(short??);


src/share/vm/classfile/classFileParser.cpp

    It is a nice refactoring with the copy_localvariable_table() !
    The function ClassFileParser::parse_method() is too big.
    It still needs more changes like this.

    I was thinking how to make the copy_method_annotations() or its
    equivalent to be more elegant but did not find anything better yet.


src/share/vm/prims/jvmtiRedefineClasses.cpp

    It is nice that all the *rewrite_cp_refs_in...annotations**()* 
became simpler!


src/share/vm/oops/constMethod.hpp

   97 // Utility class decribing elements in checked exceptions table inlined in Method*.  
  104 // Utility class decribing elements in local variable table inlined in Method*.

    A typo (was in the original version too): decribing => describing


src/share/vm/oops/instanceKlass.cpp

    // This function deallocates the metadata and C heap pointers that the
    // InstanceKlass points to.
    void InstanceKlass::deallocate_contents(ClassLoaderData* loader_data) {
   @@ -367,8 +365,18 @@
      set_init_lock(NULL);
  
      // We should deallocate the Annotations instance
   -  MetadataFactory::free_metadata(loader_data, annotations());
   -  set_annotations(NULL);
   +  if (class_annotations() != NULL) {
   +    MetadataFactory::free_array<u1>(loader_data, class_annotations());
   +  }
   +  if (class_type_annotations() != NULL) {
   +    MetadataFactory::free_array<u1>(loader_data, class_type_annotations());
   +  }
   +  if (fields_annotations() != NULL) {
   +    Annotations::free_contents(loader_data, fields_annotations());
   +  }
   +  if (fields_type_annotations() != NULL) {
   +    Annotations::free_contents(loader_data, fields_type_annotations());
   +  }
    }

    It seems to me, this line is still needed for safety:

   -  set_annotations(NULL);



src/share/vm/prims/jvm.cpp

   1576       AnnotationArray* type_annotations = InstanceKlass::cast(k)->class_type_annotations();
   1577       if (type_annotations != NULL) {
   1578         typeArrayOop a = Annotations::make_java_array(InstanceKlass::cast(k)->class_type_annotations(), CHECK_NULL);

     Not sure if I do not miss anything but why not like this? :

   1578         typeArrayOop a = Annotations::make_java_array(type_annotations, CHECK_NULL);



Thanks,
Serguei


On 1/31/13 2:12 PM, Coleen Phillimore wrote:
> Summary: allocate method annotations and attach to ConstMethod if present
>
> From the bug report:
>
> This is related to 8003419. The annotations are allocated in Metaspace 
> during class file parsing and pointers to them are carried around 
> through the parser. In order to clean these up, you'd have to collect 
> these pointers somewhere so they can be cleaned up if the parse fails.
>
> Instead, attach method annotations to the constMethod so that they can 
> be cleaned up if the ConstMethod has to be cleaned up.
>
> If any annotations exists for any method, an Array<u1> is created for 
> that method, but it's put into an Array<Array<u1>*> (an array of these 
> arrays) where there's an entry for each method in the klass, so the 
> other methods would have a pointer allocated for it whether needed or 
> not. There are 3 of these array of arrays in the type Annotations, and 
> an Annotations* object for type annotations, which are so far created 
> infrequently.
>
> The fix is to move the 4 types of method annotations to embedded 
> pointers in the ConstMethod if they are needed and add a flag to 
> indicate whether they are present. You could embed the annotations 
> directly, but the length has to be pulled out as an 'int' from 
> unaligned storage, and the bit math is getting to be too fragile 
> without a redesign.
>
> Also, some code was refactored in parse_method() and the code that 
> sorted annotation arrays to match sorted method arrays isn't needed 
> anymore.  This is in a couple of places, including defaultMethods and 
> RedefineClasses.
>
> The real purpose of this change is to make the annotations allocated 
> in Metaspace easier to clean up if class file parsing fails, but 
> hopefully has some cleanup and space saving benefits as well.
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8007320/
> bug link at http://bugs.sun.com/view_bug.do?bug_id=8007320 (just filed 
> it so might not be available yet)
>
>
> Tested with serviceability tests (nsk.sajdi.testlist, 
> vm.tmtools.testlist),  jtreg tests for annotations, including the new 
> type annotation tests, jtreg tests for java/lang/instrument, 
> nsk.parallel_class_loading.testlist and vm.quick.testlist (which is 
> the same as full).
>
> Thanks,
> Coleen
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130204/5b50b09c/attachment.html