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

Coleen Phillimore coleen.phillimore at oracle.com
Thu Mar 7 13:37:16 PST 2013

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 setinitialmethodidnum(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. copymethodordering loops on i < methods->length() and deallocatemethods loops on <= methods->length() - 1 Any chance you could simplify to have them both loop on < methodslength()

Yes, done.

3. In deallocatemethods 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 deallocatecontents, 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::printon 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 validsymbolat and validklassreferenceat

Two less references to _cp for each.

7. In sortmethods, 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 classname to classname 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 layoutfields() 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/viewbug.do?bugid=8003419 Thanks, Coleen

More information about the hotspot-dev mailing list