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

[Coleen Phillimore]

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 " _verify_count" is moved from the Klass to the 
> InstanceKlass.
>        But it is removed from the vmStructs.cpp.
>        Is it because the field is NOT_PRODUCT 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   set_name_index(0);
>    87   set_signature_index(0);
>    . . .
>    91   set_constants(NULL);
>    92   set_max_stack(0);
>    93   set_max_locals(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* 
>>>> loader_data, int length, TRAPS) {*
>>>> *+                                      int length,*
>>>> *+                                      const intStack& index_map,*
>>>> *+                                      const intStack& 
>>>> invokedynamic_map, 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           _verify_count;  // 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/view_bug.do?bug_id=8003553
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>