Review request (S) 8003553: NPG: metaspace objects should be zeroed in constructors (original) (raw)

Coleen Phillimore coleen.phillimore at oracle.com
Thu Mar 7 11:34:57 PST 2013

Thank you for reviewing the code, Serguei.

On 3/7/2013 2:18 PM, serguei.spitsyn at oracle.com wrote:

Coleen,

It is good, but a couple of questions below. Q1: The field " verifycount" is moved from the Klass to the InstanceKlass. But it is removed from the vmStructs.cpp. Is it because the field is NOTPRODUCT or it is just useless for the SA?

It was never used by the SA and doesn't really have a use in an error condition. It's used to control redundant verifies.

Q2: The following initializations are removed from the method.cpp:Method::Method() : 86 setnameindex(0); 87 setsignatureindex(0); . . . 91 setconstants(NULL); 92 setmaxstack(0); 93 setmaxlocals(0); It looks like a move in reverse direction. Are those dups?

These fields were moved to ConstMethod so are initialized in it's constructor.

Thanks! Coleen

Thanks, Serguei

On 3/7/13 4:59 AM, Coleen Phillimore wrote:

I need another reviewer for this (and two for the other change). This one is really easy. You don't need to be a Reviewer. Coleen On 3/6/2013 4:25 PM, Coleen Phillimore wrote:

Jon, Thank you for reviewing this change. On 03/06/2013 12:26 AM, Jon Masamitsu wrote: http://cr.openjdk.java.net/~coleenp/8003553/src/share/vm/oops/cpCache.cpp.udiff.html

- ConstantPoolCache ConstantPoolCache::allocate(ClassLoaderData* loaderdata, int length, TRAPS) {* + int length, + const intStack& indexmap, *+ const intStack& invokedynamicmap, TRAPS) { * Why did you not move the TRAP parameter to a new line? I think TRAPS aren't interesting enough and short enough not to merit their own line. http://cr.openjdk.java.net/~coleenp/8003553/src/share/vm/oops/methodData.cpp.udiff.html Did you remove the TieredCompilation test *if (TieredCompilation) { * for consistency of initialization? Yes, I did. I don't know if people will object but I think it's more sanitary to have these zero initialized even if they are not used in !TieredCompilation. And I had a consistency check that I took out that flagged these.

http://cr.openjdk.java.net/~coleenp/8003553/src/share/vm/oops/klass.hpp.udiff.html * #ifndef PRODUCT* * int verifycount; // to avoid redundant verifies* * #endif* Not concerned about redundant verifies anymore? I moved it from Klass* to InstanceKlass* where it's used. It still protects us from redundant verifies. Rest looks good. Thanks! Coleen Jon On 3/5/2013 11:30 AM, Coleen Phillimore wrote: Adding hotspot-dev to get some more potential reviewers from maybe the GC team, hint... Coleen On 03/04/2013 03:02 PM, Coleen Phillimore wrote: Summary: Zero metadata in constructors, not in allocation (and some in constructors) This seems like a good first step in passing initial values into constructors and no initializing metadata types by the callers (although a lot more parameters will have to be passed for some). Tested with runThese jck, NSK vm.quick.testlist, lang and vm jck8, java/lang/annotation jtreg tests and java/lang/invoke jtreg tests. open webrev at http://cr.openjdk.java.net/~coleenp/8003553/ bug link at http://bugs.sun.com/viewbug.do?bugid=8003553 Thanks, Coleen

More information about the hotspot-dev mailing list