Request for review 8007320: NPG: move method annotations
Ioi Lam
ioi.lam at oracle.com
Tue Feb 5 13:59:10 PST 2013
src/share/vm/oops/constMethod.cpp:
Are you missing these?
sz->_annotations_bytes += (a1 + a2 + a3 + a4);
sz->_ro_bytes += (a1 + a2 + a3 + a4);
- 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/20130205/7f29a9f8/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list