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

Karen Kinnear karen.kinnear at oracle.com
Thu Mar 7 12:57:32 PST 2013

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.

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

  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.

  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.

  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

  6. I like your valid_symbol_at and valid_klass_reference_at

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

  8. classFileParser.cpp line 4476 "and" -> "an"

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