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

Jon Masamitsu jon.masamitsu at oracle.com
Mon Mar 11 11:46:19 PDT 2013

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 layoutfields. I'll move it back (and remove the fac and parsedallocations copying) after the review. http://cr.openjdk.java.net/~coleenp/8003419small/ This is much easier to read! Thanks. This helps. The description for applyparsedclassmetadata() is *+ // Transfer ownership of metadata allocated to the InstanceKlass. When applyparsedclassmetadata() 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 mapcount = superklass->nonstaticoopmapcount(); Yes, I changed it. The field nonstaticoopmapcount 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 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