Request for Review (s) - 8014862:Add fast Metasapce capacity and used per MetadataType

[Stefan Karlsson]

On 20 maj 2013, at 17:56, Jon Masamitsu <jon.masamitsu at oracle.com> wrote:

> Stefan,
> 
> Thanks for the review.
> 
> On 5/20/13 2:10 AM, Stefan Karlsson wrote:
>> On 05/20/2013 07:40 AM, Jon Masamitsu wrote:
>>> This fix calculates the running sums for allocated capacity and
>>> allocated used in MetaspaceAux to be  per MetaspaceType instead of
>>> the sum of both types.
>>> 
>>> http://cr.openjdk.java.net/~jmasa/8014862/webrev.00/
>> 
>> I think this split looks mostly good. I have a few comments.
>> 
>> http://cr.openjdk.java.net/~jmasa/8014862/webrev.00/src/share/vm/memory/metaspace.cpp.frames.html 
>> 
>> 2497     err_msg("About to decrement below 0: words " SIZE_FORMAT
>> 2498             " is greater than _allocated_capacity_words[%u] " SIZE_FORMAT,
>> 2499             mdtype, words, allocated_capacity_words(mdtype)));
>> 
>> I think the order of mdtypes and words is wrong.
> 
> Agreed.  Fixed.
> 
>> 
>> 
>> 2511     err_msg("About to decrement below 0: words " SIZE_FORMAT
>> 2512             " is greater than _allocated_used_words " SIZE_FORMAT,
>> 2513             words, allocated_used_words(mdtype)));
>> 
>> This err_msg line doesn't say what mdtype is (like you do in the err_msg above).
>> 
>> 2630 // The calls to capacity_bytes_slow() and used_bytes_slow() cause
>> 2631 // lock ordering assertion failures with some collectors.  Do
>> 2632 // not include this code until the lock ordering is fixed.
>> 
>> This comment should be removed.
> 
> Done.
> 
>> 
>> 
>> http://cr.openjdk.java.net/~jmasa/8014862/webrev.00/src/share/vm/memory/metaspace.hpp.patch 
>> 
>> -  enum MetadataType {ClassType, NonClassType};
>> +  enum MetadataType {ClassType = 0,
>> +             NonClassType = ClassType + 1,
>> +             ClassTypeCount = ClassType + 2
>> +  };
>> 
>> Is there a reason why you had to explicitly enumerate the values here?
> 
> I understood that compilers are allowed to assign any distinct values to
> the enums.   So here I make sure they are consecutive.

I doubt this is true. If this is the case then we have a lot of potential bugs in the code.

StefanK

> 
>> 
>> ClassTypeCount doesn't seem to be an appropriate name. Maybe use MetadataTypeCount instead?
> 
> Using MetadataTypeCount now
> 
> Jon
> 
>> 
>> thanks,
>> StefanK
>