Request for review 8007320: NPG: move method annotations
Coleen Phillimore
coleen.phillimore at oracle.com
Wed Feb 6 05:40:23 PST 2013
Thanks Serguei for the additional review.
Coleen
On 2/5/2013 6:35 PM, serguei.spitsyn at oracle.com wrote:
> On 2/5/13 2:45 PM, serguei.spitsyn at oracle.com wrote:
>> Hi Coleen,
>>
>>
>> Thank you for making the changes!
>>
>> Just a couple of comments below.
>>
>>
>> src/share/vm/oops/constMethod.cpp:
>>
>> 221 void ConstMethod::set_inlined_tables_length(InlineTableSizes* sizes) {
>> 222 _flags = 0;
>> 223 if (sizes->compressed_linenumber_size() > 0)
>> 224 _flags |= _has_linenumber_table;
>> * 225 if (sizes->generic_signature_index() != 0)*
>> 226 _flags |= _has_generic_signature;
>> 227 if (sizes->method_parameters_length() > 0)
>> 228 _flags |= _has_method_parameters;
>> 229 if (sizes->checked_exceptions_length() > 0)
>> 230 _flags |= _has_checked_exceptions;
>> 231 if (sizes->exception_table_length() > 0)
>> 232 _flags |= _has_exception_table;
>> 233 if (sizes->localvariable_table_length() > 0)
>> 234 _flags |= _has_localvariable_table;
>>
>> It was so in original version but would better to make it consistent
>> with other conditions:
>> * 225 if (sizes->generic_signature_index() > 0)*
>
>
> I'm taking back my suggestion.
> This is an index, not length, so it is better to keep it the way it is.
>
>
> Thanks,
> Serguei
>
>>
>> 378 void ConstMethod::collect_statistics(KlassSizeStats *sz) const {
>> 379 int n1, n2, n3;
>> 380 sz->_const_method_bytes += (n1 = sz->count(this));
>> 381 sz->_bytecode_bytes += (n2 = code_size());
>> 382 sz->_stackmap_bytes += (n3 = sz->count_array(stackmap_data()));
>> 383
>> 384 // Count method annotations
>> 385 int a1 = 0, a2 = 0, a3 = 0, a4 = 0;
>>
>> As Ioi already pointed out the values for the a1-a4 are calculated
>> but not used.
>>
>>
>> The rest looks good.
>>
>>
>> Thanks,
>> Serguei
>>
>>
>> On 2/5/13 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/80997571/attachment.html
More information about the hotspot-runtime-dev
mailing list