Request for review 8007320: NPG: move method annotations
Coleen Phillimore
coleen.phillimore at oracle.com
Wed Feb 6 04:11:21 PST 2013
On 2/5/2013 4:59 PM, Ioi Lam wrote:
> src/share/vm/oops/constMethod.cpp:
>
> Are you missing these?
>
> sz->_annotations_bytes += (a1 + a2 + a3 + a4);
> sz->_ro_bytes += (a1 + a2 + a3 + a4);
>
Yes, I am. Thanks for spotting this.
Coleen
> - Ioi
>
>
> 384 // Count method annotations
> 385 int a1 = 0, a2 = 0, a3 = 0, a4 = 0;
> 386 if (has_method_annotations()) {
> 387 sz->_methods_annotations_bytes += (a1 = sz->count_array(method_annotations()));
> 388 }
> 389 if (has_parameter_annotations()) {
> 390 sz->_methods_parameter_annotations_bytes += (a2 = sz->count_array(parameter_annotations()));
> 391 }
> 392 if (has_type_annotations()) {
> 393 sz->_methods_type_annotations_bytes += (a3 = sz->count_array(type_annotations()));
> 394 }
> 395 if (has_default_annotations()) {
> 396 sz->_methods_default_annotations_bytes += (a4 = sz->count_array(default_annotations()));
> 397 }
> 398
>
>
>
>
> On 02/05/2013 10:33 AM, Coleen Phillimore wrote:
>>
>> Thanks Serguei,
>>
>> I have changes from merging with Ioi's class statistic dumping and a
>> change that you requested below in webrev:
>>
>> http://oklahoma.us.oracle.com/~cphillim/webrev/8007320_2/
>>
>>
>> On 02/04/2013 10:04 PM, serguei.spitsyn at oracle.com wrote:
>>> 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??);
>>
>> Dan had the same comment. There are a lot of -2's in this code so I
>> didn't want to change one and not the others, and I didn't want to
>> change them all in the SA. It is supposed to be sizeof(short). I
>> declared a const sizeofU2 in the Java code. See:
>>
>>
>>
>>>
>>> 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.
>>
>> Yes, a method with so many parameters is not good, but I think still
>> better than inlined. I think incremental refactoring of these
>> functions is needed.
>>
>>>
>>>
>>> src/share/vm/prims/jvmtiRedefineClasses.cpp
>>>
>>> It is nice that all the *rewrite_cp_refs_in...annotations**()*
>>> became simpler!
>>>
>>
>> I think this is worth the edit on it's own, even though I need it for
>> another bug.
>>>
>>> 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
>>>
>>
>> Oops, I got one but not the other.
>>>
>>> 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);
>>>
>>
>> This change was a mistake. I needed to free the Annotations type not
>> the annotation arrays individually. It was left over from a change I
>> made to embed the annotations in instanceKlass (which I reverted).
>> Thanks! I reverted this change too.
>>
>>>
>>> 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);
>>>
>> You are right. Fixed.
>>
>> Thanks!
>> Coleen
>>
>>>
>>> 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/20130206/52d75438/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list