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

[Coleen Phillimore]

On 3/11/2013 2:46 PM, Jon Masamitsu wrote:
> Coleen,
>
> Looks good.

Thank you, Jon!
>
> Humanity will thank you for the refactoring
> of parseClassFile() as much as you did.

I hope the C++ compiler likes it too.

Thanks,
Coleen

>
> Jon
>
> On 03/11/13 10:57, Coleen Phillimore wrote:
>> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>