Request for review 8007320: NPG: move method annotations
Coleen Phillimore
coleen.phillimore at oracle.com
Mon Feb 4 17:29:09 PST 2013
One short note below.
On 2/4/2013 8:08 PM, Coleen Phillimore wrote:
> On 2/2/2013 12:05 AM, Daniel D. Daugherty wrote:
>> > open webrev at http://cr.openjdk.java.net/~coleenp/8007320/
>>
>> agent/src/share/classes/sun/jvm/hotspot/oops/ConstMethod.java
>> line 435: return (getSize() * wordSize) - (offset * wordSize) - 2;
>> Should that literal '2' be something like sizeof(short)? I see
>> bunch of other literal '2's in near by code so looks like that
>> is probably the style for this code.
>
> Yes, it's backing up to sizeof(short) in all these cases. I don't
> know if I should just change this one without changing the others and
> I don't want to change too much in SA.
>
>>
>> src/share/vm/classfile/classFileParser.cpp
>> line 1872: void ClassFileParser::copy_localvariable_table(
>> I wasn't expecting LVT work with this bug report. I think
>> Serguei
>> was the last person to do work on LVT/LVTT stuff so you'll want
>> to make sure he gets a look at this new code.
>
> I did slip in a refactoring here because I kept losing the my way to
> the end of parse_method() where the annotation code was, and this was
> easily separable. I only moved this code to it's own function. I
> only made two minor modifications to get the constants from the
> ConstMethod passed in.
>
>>
>> nit line 1893: if (LVT_put_after_lookup(lvt, lvt_Hash) == false
>> nit line 1894: && _need_verify
>> nit line 1895: && _major_version >= JAVA_1_5_VERSION ) {
>> Usually, a continued if-statement lines up just inside
>> the 'if (' like this:
>> nit line 1893: if (LVT_put_after_lookup(lvt, lvt_Hash) == false
>> nit line 1894: && _need_verify
>
> I will reformat this as I've already moved this code. I probably
> missed reindenting the if clauses. I like the && || expressions at
> the end but I don't think we have a coding standard for that.
>
>>
>>
>> old line 4047: host_klass,
>> new line 4073: !host_klass.is_null(),
>> The above changed in a call to
>> InstanceKlass::allocate_instance_klass()
>> but the reason isn't obvious yet.
>
> Passing a boolean seemed more consistent, so I changed it.
>>
>> Update: OK, the changes in instanceKlass.cpp make things more
>> clear.
>>
>> src/share/vm/classfile/classFileParser.hpp
>> No comments.
>>
>> src/share/vm/classfile/defaultMethods.cpp
>> old line 1150: Method* m =
>> Method::allocate(cp->pool_holder()->class_loader_data(),
>> old line 1151: code_length, flags,
>> 0, 0, 0, 0, 0, 0,
>>
>> new line 1149: InlineTableSizes sizes;
>> new line 1151: Method* m =
>> Method::allocate(cp->pool_holder()->class_loader_data(),
>> new line 1152: code_length, flags,
>> &sizes,
>> In the old code, zero's were passed in. In the new code, "sizes"
>> is uninitialized. Is that OK? See comments in constMethod.cpp
>> below.
>
> Sizes are initialized to zero by the macro.
>>
>> Does line 1149 need to be: "InlineTableSizes sizes();" in
>> order to
>> get the default constructor stuff to work? Sorry, my C++
>> constructor
>> memory is faulty... :-)
>
> No, it should call the default constructor without (). I'll double
> check, now you have me worried about my memory.
Yes, I verified that the default constructor is called which zeros the
sizes passed into the ConstMethod allocator.
Coleen
>>
>> src/share/vm/oops/annotations.cpp
>> No comments.
>>
>> src/share/vm/oops/annotations.hpp
>> No comments.
>>
>> src/share/vm/oops/constMethod.cpp
>> line 36: ConstMethod* ConstMethod::allocate(ClassLoaderData*
>> loader_data,
>> line 37: int byte_code_size,
>> line 38: InlineTableSizes* sizes,
>> line 41: int size = ConstMethod::size(byte_code_size, sizes);
>> The old call to Method::allocate() in defaultMethods.cpp
>> passed zeros.
>> The new call to Method::allocate() in defaultMethods.cpp
>> passes a
>> 'sizes' obj, but I don't think it is initialized.
>>
>> It looks like ConstMethod::size() is using the 'sizes' object
>> that was passed in.
>>
>
> Same, will double check the constructor that zeros it is called.
>>
>> src/share/vm/oops/constMethod.hpp
>> nit lines 139-148: indent appears to be 3
>> nit lines 157-159: indent appears to be 3
>> nit lines 161-172: indent appears to be 3, and multiples of 3
>
> You are right. I fixed them.
>>
>> lines 190-200: Thanks for switching to HEX; easier for my brain
>> anyway.
>
> My brain greatly prefers:
>
> u2
> _has_linenumber_table : 1,
> _has_checked_exceptions : 1,
> _has_localvariable_table : 1,
> _has_exception_table : 1,
> _has_generic_signature : 1,
> _has_method_parameters : 1,
> _is_overpass : 1,
> _has_method_annotations : 1,
> _has_parameter_annotations : 1,
> _has_type_annotations : 1,
> _has_default_annotations : 1,
> : 5; // flags left
>
> And let the compilers do bit manipulation like they've been doing for
> years, but the duplication of this code can't be coded in the
> serviceability agent.
>
>>
>> src/share/vm/oops/instanceKlass.cpp
>> No comments.
>>
>> src/share/vm/oops/instanceKlass.hpp
>> No comments.
>>
>> src/share/vm/oops/method.cpp
>> new line 1021: InlineTableSizes sizes;
>> new line 1022: Method* m_oop = Method::allocate(loader_data, 0,
>> new line 1023: accessFlags_from(flags_bits), &sizes,
>> The "sizes" object appears to be uninitialized.
>>
>> Does line 1021 need to be: "InlineTableSizes sizes();" in
>> order to
>> get the default constructor stuff to work?
>
> Ditto above comment.
>>
>> src/share/vm/oops/method.hpp
>> No comments.
>>
>> src/share/vm/prims/jvm.cpp
>> No comments.
>>
>> src/share/vm/prims/jvmtiRedefineClasses.cpp
>> No comments. I was going to ask about the removal of the tracing
>> of the various annotations lengths, but I think they're all the
>> same size as the methods() array now.
>
> Yes, that traced length is always the same as the methods->length().
> The function rewrite_cp_refs_in_annotations_typeArray prints the
> lengths of the annotations.
>
> 1621 RC_TRACE_WITH_THREAD(0x02000000, THREAD,
> 1622 ("num_annotations=%d", num_annotations));
>
>>
>>
>> src/share/vm/prims/jvmtiRedefineClasses.hpp
>> No comments.
>>
>> src/share/vm/runtime/fieldDescriptor.cpp
>> No comments.
>>
>> src/share/vm/runtime/vmStructs.cpp
>> No comments.
>>
>
> thanks!
> Coleen
>
>> Dan
>>
>>
>> On 1/31/13 3: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
>>
>
More information about the hotspot-runtime-dev
mailing list