Request for review 8007320: NPG: move method annotations

[Daniel D. Daugherty]

 > open webrev at http://cr.openjdk.java.net/~coleenp/8007320/

agent/src/share/classes/sun/jvm/hotspot/oops/ConstMethod.java
     line 435: return (getSize() * wordSize) - (offset * wordSize) - 2;
         Should that literal '2' be something like sizeof(short)? I see
         bunch of other literal '2's in near by code so looks like that
         is probably the style for this code.

src/share/vm/classfile/classFileParser.cpp
     line 1872: void ClassFileParser::copy_localvariable_table(
         I wasn't expecting LVT work with this bug report. I think Serguei
         was the last person to do work on LVT/LVTT stuff so you'll want
         to make sure he gets a look at this new code.

     nit line 1893:  if (LVT_put_after_lookup(lvt, lvt_Hash) == false
     nit line 1894:    && _need_verify
     nit line 1895:    && _major_version >= JAVA_1_5_VERSION ) {
         Usually, a continued if-statement lines up just inside
         the 'if (' like this:
     nit line 1893:  if (LVT_put_after_lookup(lvt, lvt_Hash) == false
     nit line 1894:      && _need_verify

     old line 4047:  host_klass,
     new line 4073:  !host_klass.is_null(),
        The above changed in a call to 
InstanceKlass::allocate_instance_klass()
        but the reason isn't obvious yet.

        Update: OK, the changes in instanceKlass.cpp make things more clear.

src/share/vm/classfile/classFileParser.hpp
     No comments.

src/share/vm/classfile/defaultMethods.cpp
     old line 1150:   Method* m = 
Method::allocate(cp->pool_holder()->class_loader_data(),
     old line 1151:                                code_length, flags, 
0, 0, 0, 0, 0, 0,

     new line 1149:   InlineTableSizes sizes;
     new line 1151:   Method* m = 
Method::allocate(cp->pool_holder()->class_loader_data(),
     new line 1152:                                code_length, flags, 
&sizes,
         In the old code, zero's were passed in. In the new code, "sizes"
         is uninitialized. Is that OK? See comments in constMethod.cpp 
below.

         Does line 1149 need to be: "InlineTableSizes sizes();" in order to
         get the default constructor stuff to work? Sorry, my C++ 
constructor
         memory is faulty... :-)

src/share/vm/oops/annotations.cpp
     No comments.

src/share/vm/oops/annotations.hpp
     No comments.

src/share/vm/oops/constMethod.cpp
     line 36: ConstMethod* ConstMethod::allocate(ClassLoaderData* 
loader_data,
     line 37:                                    int byte_code_size,
     line 38:                                    InlineTableSizes* sizes,
     line 41:   int size = ConstMethod::size(byte_code_size, sizes);
         The old call to Method::allocate() in defaultMethods.cpp passed 
zeros.
         The new call to Method::allocate() in defaultMethods.cpp passes a
         'sizes' obj, but I don't think it is initialized.

         It looks like ConstMethod::size() is using the 'sizes' object
         that was passed in.


src/share/vm/oops/constMethod.hpp
     nit lines 139-148: indent appears to be 3
     nit lines 157-159: indent appears to be 3
     nit lines 161-172: indent appears to be 3, and multiples of 3

     lines 190-200: Thanks for switching to HEX; easier for my brain anyway.

src/share/vm/oops/instanceKlass.cpp
     No comments.

src/share/vm/oops/instanceKlass.hpp
     No comments.

src/share/vm/oops/method.cpp
     new line 1021:     InlineTableSizes sizes;
     new line 1022:     Method* m_oop = Method::allocate(loader_data, 0,
     new line 1023:                                      
accessFlags_from(flags_bits), &sizes,
         The "sizes" object appears to be uninitialized.

         Does line 1021 need to be: "InlineTableSizes sizes();" in order to
         get the default constructor stuff to work?

src/share/vm/oops/method.hpp
     No comments.

src/share/vm/prims/jvm.cpp
     No comments.

src/share/vm/prims/jvmtiRedefineClasses.cpp
     No comments. I was going to ask about the removal of the tracing
     of the various annotations lengths, but I think they're all the
     same size as the methods() array now.

src/share/vm/prims/jvmtiRedefineClasses.hpp
     No comments.

src/share/vm/runtime/fieldDescriptor.cpp
     No comments.

src/share/vm/runtime/vmStructs.cpp
     No comments.

Dan


On 1/31/13 3: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