Request for review 8007320: NPG: move method annotations
Coleen Phillimore
coleen.phillimore at oracle.com
Tue Feb 5 10:33:44 PST 2013
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/20130205/9d6555a8/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list