Request for review 8007320: NPG: move method annotations
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Feb 5 15:35:08 PST 2013
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/20130205/d3c4a021/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list