Request for review 8007320: NPG: move method annotations

[Jiangli Zhou]

Hi Coleen,

Sounds good to me.

Thanks,
Jiangli

On 01/31/2013 07:18 PM, Coleen Phillimore wrote:
>
> 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
>>>
>>>
>>