Request for review 8007320: NPG: move method annotations
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Feb 5 14:45:56 PST 2013
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)*
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/549651ff/attachment.html
More information about the hotspot-runtime-dev
mailing list