Request for review (m) 8008966: NPG: Inefficient Metaspace counter functions cause large young GC regressions

[Jon Masamitsu]

On 3/27/2013 2:14 PM, Coleen Phillimore wrote:
>
> Hi Jon,
>
> In metaspace.hpp maybe capacity_in_bytes should be called 
> capacity_bytes_slow() or something like that to distinguish it from 
> capacity_bytes() which is fast.
>
> *+   // Total capacity in all Metaspaces*
>     static size_t capacity_in_bytes() {
> *       return capacity_in_bytes(Metaspace::ClassType) +*
> *              capacity_in_bytes(Metaspace::NonClassType);*
> *+  #ifdef PRODUCT*
> *+      // Use capacity_bytes() in PRODUCT instead of this function.*
> *+     guarantee(false, "Should not call capacity_in_bytes() in the 
> PRODUCT");*
> *+ #endif
>
> *

I like the name capacity_bytes_slow() so made the change 
capacity_in_bytes -> capacity_bytes_slow
>
> Or maybe since capacity_in_bytes() is used for the other counters, 
> change the capacity_bytes() name to allocated_capacity() or something 
> like that.   Just so the two names are more different than the 
> presence of "_in".

I will keep this second suggestion in mind.  I'm going to be making some 
other name changes
so may use allocated_capacity.

>
> metaspace.cpp:
>
> line 270 and 283 are missing an 'n' in accounting.   I like the 
> promise of a cleanup.   Even with the comment, it's hard to keep these 
> straight.
>
> 1199-1201 is the same code as above it.

Fixed.

>
> 2520 capacity_in_bytes(mdtype) is still called for PrintGCDetails 
> which iterates over the CLD graph.   This seems too expensive for GC 
> printing.   It also calls used_in_bytes() so iterates twice. Then x2 
> for class vs. data metaspace.   This wasn't part of the GC slowdown 
> that was observed?

Yes this this is part of the slow down.  I don't have a replacement yet 
for used_in_bytes() yet so thought I
would fix this when I replaced used_in_bytes().   I can diff out this code
(the code that does data_space and class_space output separately) or
I can fix it.  Should I just fix it now?

>
> 2935.  I don't understand why we are checking UseMallocOnly since we 
> don't use malloc for metaspaces ever.

That was probably a mis-merge when I started with the malloc-chunks 
patch.  I'll
delete it.

>
> I can't comment on the CMS change.  It looks like you just moved it.

Yes, I just moved a class up so that I could use it in the "Resizing" phase.

Thanks.

Jon

>
> Coleen
>
>
> On 3/27/2013 12:13 AM, Jon Masamitsu wrote:
>>
>> Replace the use of a method that calculated the total capacity of
>> the Metaspaces by iterating over all the Metaspaces by maintaining
>> the sum of Metaspace capacities as the Metachunks are
>> allocated to each Metaspace.  Also maintain a sum for each
>> Metaspace as the Metachunks are allocated to that Metaspace.
>>
>> Change should_expand() and compute_new_space() to
>> calculate quantities in bytes.
>>
>> Remove calls to methods that calculated totals for
>> "used" and "free" by iterating over the Metaspaces
>> in product mode.  In some cases substitute the use
>> of capacity for used.
>>
>> http://cr.openjdk.java.net/~jmasa/8008966/webrev.00/
>>
>> Thanks.
>>
>> Jon
>
>