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

[Jon Masamitsu]

Coleen,

Looks good.

Humanity will thank you for the refactoring
of parseClassFile() as much as you did.

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