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

[Coleen Phillimore]

Karen,

Thank you for reviewing this code.

On 3/7/2013 3:57 PM, Karen Kinnear wrote:
> Coleen,
>
> Thank you so much for doing this and for the refactoring!
>
> A couple of minor questions/comments:
>
> 1. Did you remove set_initial_method_idnum(0) in the constructor on purpose?
> I had a memory that there was check for this being 0 somewhere, but maybe
> that is handled when you set it to methods->length() if the length is 0.

That was a mistake separating the initializations to the other 
changeset.  I reverted it.
>
> 2. copy_method_ordering loops on i < methods->length() and
> deallocate_methods loops on <= methods->length() - 1
> Any chance you could simplify to have them both loop on < methods_length()

Yes, done.

> 3. In deallocate_methods
> If the method is NULL, did you want break or continue?
> I was concerned if we had a method that was NULL (due to an error) but others were
> not null, that they would not get freed, but we would free the array so we would lose track of them.

The last element would be the one that got the error so the others will 
be null, but for sanity sake, I changed it to continue.
> 4. in deallocate_contents, can you add one more change that I ended up needing please?
> Please add an if (constants() != NULL) before the assert "shouldn't be called if anything ..."
> I'll do this if you don't, so no need to slow you down.

Yes added that.
> 5. You don't feel like improvements to instanceKlass::print_on do you?
> i.e. some more if (xxx != 0) lines?
> If not - no worries, I'll add some when my changes go in

Someone added print_value_on_maybe_null() which is odd.  The oop 
versions of print_value_on() check if this == NULL which the metadata 
versions should also do.   I didn't want to make this change now because 
it's in a different file.

> 6. I like your valid_symbol_at and valid_klass_reference_at

Two less references to _cp for each.

>
> 7. In sort_methods, does new intArray throw on exception?

No, it's resource allocated.  Eventually it might throw OOM or do 
something else if we can't get another thread local Arena.
> 8. classFileParser.cpp line 4476 "and" -> "an"
Got it.

Thanks for doing the code review!
Coleen
> thanks,
> Karen
>
>
> On Mar 5, 2013, at 2:30 PM, 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
>>>