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

[Coleen Phillimore]

On 3/11/2013 1:43 PM, Jon Masamitsu wrote:
>
>
> 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?

Yes.
Coleen

>
> 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
>>>>>
>>>>
>>>
>>