Review request (L) 8003419: NPG: Clean up metadata created during class loading if failure

[Jon Masamitsu]

On 03/09/13 07:01, Coleen Phillmore wrote:
>
> Hi Jon,
>
> Thank you for starting to look at this.   The huge number of diffs in 
> classFileParser.cpp is because I moved the code that laid out fields 
> and diff matches oddly so it looks like I changed a lot more than I 
> did.   I should have done this in the first place, but here are the 
> changes I made without moving layout_fields.   I'll move it back (and 
> remove the fac and parsed_allocations copying) after the review.
>
> http://cr.openjdk.java.net/~coleenp/8003419_small/
>
> This is much easier to read!

Thanks.  This helps.

The description for apply_parsed_class_metadata() is

*+  // Transfer ownership of metadata allocated to the InstanceKlass.

When apply_parsed_class_metadata() all the metadata for
the Klass has been allocated and is hanging off the
ClassFileParser?

Jon

*



>
> On 3/8/2013 7:19 PM, Jon Masamitsu wrote:
>> Coleen,
>>
>> http://cr.openjdk.java.net/~coleenp/8003419/src/share/vm/classfile/classFileParser.hpp.udiff.html 
>>
>>
>> *+    // metadata created before the instance klass is created. Must be*
>> *+   // deallocated if classfile parsing returns an error.*
>>
>> Could this also have said
>>
>> "Must be deallocated if not transferred to the InstanceKlass upon 
>> sucessful class loading
>> in which case these pointers have been set to NULL."
>
> I changed the comment (adding a 'c' to it) to yours.
>>
>> At first I didn't understand why the destructor didn't always 
>> deallocate the
>> metadata.
>>
>> Can all the fields with pointers to metadata be grouped together
>> and in the destructor iterate through the pointers to see that they are
>> all NULL?  Worth doing to catch leaks?
>
> The deallocation uses template functions for the types of each pointer 
> so iterating through pointers won't work.
>>
>>
>> This file just had so many changes in it I quickly bogged down. I'll
>> try again later.  So far only
>>
>> http://cr.openjdk.java.net/~coleenp/8003419/src/share/vm/classfile/classFileParser.cpp.udiff.html 
>>
>>
>>   2991    // Assign allocations if needed
>>
>> allocations or annotations?
>>
>
> Annotations, fixed.
>> Can you make this unsigned?
>>
>> 3288       int map_count = _super_klass->nonstatic_oop_map_count();
>>
>
> Yes, I changed it.  The field nonstatic_oop_map_count is unsigned.
>
> Thanks!
> Coleen
>
>>
>>
>> Jon
>>
>>
>> On 3/5/2013 11:30 AM, Coleen Phillimore wrote:
>>>
>>> Resend to hotspot-dev.   I don't think anyone reads 
>>> hotspot-runtime-dev anymore.
>>> Please review!!
>>> Thanks,
>>> Coleen
>>>
>>> On 03/04/2013 03:13 PM, Coleen Phillimore wrote:
>>>> Summary: Store metadata on ClassFileParser instance to be cleaned 
>>>> up by destructor.  This enabled some refactoring of the enormous 
>>>> parseClassFile function.
>>>>
>>>> Note: I moved out code that lays out the java object into it's own 
>>>> function, but the diffs made it look like I made more than minor 
>>>> changes.  The changes I made was changing class_name to _class_name 
>>>> and cp to _cp, and not more than that.   To see these changes, 
>>>> you'll have to look at before/after code and not the diffs.  I also 
>>>> tried moving this layout_fields() function around to it wouldn't 
>>>> false match but I wanted to keep it next to parseClassFile() and 
>>>> not move it too far away.
>>>>
>>>> Tested with runThese jck, jck8 lang and vm, NSK vm.quick.testlist, 
>>>> java/lang/invoke jtreg tests and java/lang/instrument jtreg tests.
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8003419/
>>>> bug link at http://bugs.sun.com/view_bug.do?bug_id=8003419
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>
>>
>