Request for review 8007320: NPG: move method annotations
Coleen Phillimore
coleen.phillimore at oracle.com
Mon Feb 4 17:08:43 PST 2013
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.
>
> 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