Request for review 8007320: NPG: move method annotations

[Coleen Phillimore]

Hi Jiangli,

I was afraid someone (likely you!) would ask why I didn't embed the 
annotation arrays directly.   I could have done it but the code was 
getting difficult and bit-twiddly, and for the unlikely case of each 
annotation, it only cost an extra pointer to write less code.   So I 
took the easy route.  It could be enhanced as you suggest.  To embed the 
annotations, they count backwards from last_u2_element like all the 
other tables, which gives one of these length_addr functions a 7 deep 
nested if statement.  Ugh!

So I'd like to leave the embedding the annotations as an enhancement to 
this change.  It probably wouldn't be bad because it's only changing the 
internals of ConstMethod.   I have to integrate this with Ioi's class 
histogram dumping and I'd like to see how much more savings we can get 
first.

Thanks!
Coleen

On 1/31/2013 7:02 PM, Jiangli Zhou wrote:
> Hi Coleen,
>
> Looks great! It also simplified merge_in_new_methods(). :)
>
> I wonder if the AnnotationArray pointers could be eliminated by 
> embedding the raw annotation data in ConstMethod directly. The 
> ConstMethod::_flags could still be kept as 8-bit, with one bit to 
> indicate if the method has any type of the annotations. The embedded 
> raw annotation data would look like following in the table. Each raw 
> annotation data is preceded by it's type and size, with type being 
> 1-byte and size being 4-byte. That way there is no need to explicitly 
> free the method annotation arrays, and also reduces fragmentation. 
> There is no extra cost for the 4-byte 'size' in the embedded table for 
> each annotation, as Array would also has an int field for the size.
>
> ...
>
> |------------------------------------------------------|
> | generic signature index (u2)                         |
> |  (indexed from start of constMethodOop)              |
> |------------------------------------------------------|
> | method annotations:                                  |
> | type|size|method_annotations raw data|type|size|     |
> | parameter_annotations raw data|...                   |
> --------------------------------------------------------
>
> Thanks,
> Jiangli
>
> On 01/31/2013 02: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
>>
>>
>