Request for review 8007320: NPG: move method annotations
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Feb 8 09:26:57 PST 2013
Hi, Despite running every test I know of with fastdebug, the method
annotation changes had a memory stomp, which wasn't discovered until
JPRT tested it with product. The stomp was in the address calculation
for the annotation addr() functions in constMethod.cpp. fastdebug (and
debug) has a memory buffer between metadata which product does not. I
also added a test to create a method with annotations, method
parameters, generic_signature, localvariable_table, checked_exceptions
and exception table to test that all the optional fields are laid out
correctly.
See: http://cr.openjdk.java.net/~coleenp/8007320_3/
Reran reflection, annotation, vm.quick.testlist, lang and vm jck tests,
and hotspot jtreg tests with product vm.
Please rereview and let me know if you have any suggestions for
improving the test.
Thanks,
Coleen
On 2/6/2013 8:40 AM, Coleen Phillimore wrote:
>
> 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/20130208/f66137c7/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list