Request for review 8007320: NPG: move method annotations
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Feb 11 09:15:43 PST 2013
> See: http://cr.openjdk.java.net/~coleenp/8007320_3/
Thumbs up. Only nits or questions here.
agent/src/share/classes/sun/jvm/hotspot/oops/ConstMethod.java
line 57: private static int sizeofShort = 2;
Nice addition. Perhaps should also be "final"?
src/share/vm/classfile/classFileParser.cpp
nit line 1871: */
Need one more space for proper indent.
nit line 1895: && _major_version >= JAVA_1_5_VERSION ) {
No space before ')'. Yes, I see that came from the original code.
src/share/vm/classfile/classFileParser.hpp
No comments.
src/share/vm/classfile/defaultMethods.cpp
No comments.
src/share/vm/memory/heapInspection.hpp
No comments.
src/share/vm/oops/annotations.cpp
No comments.
src/share/vm/oops/annotations.hpp
No comments.
src/share/vm/oops/constMethod.cpp
line 111 // Align sizes up to a pointer word.
line 112 extra_bytes = align_size_up(extra_bytes, BytesPerWord);
I found the "up to a pointer word" to be a little confusing.
I was expecting there to be something like BytesPerPtrWord much
like intptr_t, but there isn't so perhaps "up to a word" would
be less confusing unless there is a reason you have "pointer"
in there...
src/share/vm/oops/constMethod.hpp
nit line 134 #define INLINE_TABLES_DO(do_element) \
Indent before backslash off by one space.
src/share/vm/oops/instanceKlass.cpp
No comments.
src/share/vm/oops/instanceKlass.hpp
No comments.
src/share/vm/oops/method.cpp
No comments.
src/share/vm/oops/method.hpp
No comments.
src/share/vm/prims/jvm.cpp
No comments.
src/share/vm/prims/jvmtiRedefineClasses.cpp
No comments.
src/share/vm/prims/jvmtiRedefineClasses.hpp
No comments.
src/share/vm/runtime/fieldDescriptor.cpp
No comments.
src/share/vm/runtime/vmStructs.cpp
No comments.
test/runtime/8007320/ConstMethodTest.java
line 122: check(foo.equals(foo));
If you are checking that "foo" equals itself, then shouldn't
you also check:
check(bar.equals(bar));
line 100: @ScalarTypesWithDefault T x)
line 143: kitchenSinkFunc("parameter", "param2", 5);
Looks like literal "5" is passed as parameter "x", but "x"
is unused in kitchenSinkFunc(). Does that parameter exist
just so you can associate an annotation with it? Might be
worth a comment. Perhaps:
// pass various parameters so we can associate annotations
kitchenSinkFunc("parameter", "param2", 5);
line 142: // Not sure what to do yet.
stale comment?
Dan
On 2/8/13 10:26 AM, Coleen Phillimore wrote:
>
> 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/20130211/9c8b141f/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list