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

[Mikael Gerdin]

Jon,

On 2013-04-19 07:03, Jon Masamitsu wrote:
> This is the webrev for the complete set of  changes
> (updated to the latest gc_baseline).
>
> http://cr.openjdk.java.net/~jmasa/8008966/webrev.01/

It looks like you've changed the value of 
MetaspaceGC::_capacity_until_GC to be in bytes.
In that case you should remove MetaspaceGC::capacity_until_GC_in_bytes() 
and change Metaspace::expand_and_allocate which still seems to call 
inc_capacity_until_GC with a size in words.

I'm curious about
1930   if (vs_list() != NULL) {
1931     vs_list()->chunk_manager()->locked_print_free_chunks(st);
1932     vs_list()->chunk_manager()->locked_print_sum_free_chunks(st);
1933   }

I'm having trouble understanding how _vs_list can be null since it's 
initialized in the SpaceManager constructor. Did you see a SpaceManager 
with an uninitialized _vs_list?


2657              capacity_bytes / K);
2658   assert(used_and_free == capacity_bytes, "Accounting is wrong");

2659 }
You removed the safepoint case here, the reason for that code is that
Metaspace::allocate calls MetaspaceAux::dump() when not at safepoint if 
+Verbose && +TraceMetadataChunkAllocation before throwing OOME.
If another thread is allocating metadata and not failing while this 
function is being executed we can get inconsistent accounting, right?

If you don't want the !SafepointSychronize::is_at_safepoint() condition 
in the assert then please remove the call to MetasapceAux::dump from the 
allocate path.

The rest looks good.

/Mikael


>
> Thanks.
>
> Jon
>
> On 4/12/2013 4:04 PM, Jon Masamitsu wrote:
>> This is the promised semantic changes for the code review.  Whereas
>> the original set of changes only maintained a running sum for the
>> capacity of all the space for metadata, this added a running sum
>> for the used space allocated and holding metadata.
>>
>> http://cr.openjdk.java.net/~jmasa/8008966/webrev.00_2/
>>
>> This is an incremental webrev based on the name change webrev
>> (webrev.00_1) below.  A complete webrev with all changes will
>> be published after comments for this have been digested.
>>
>> Thanks.
>>
>> Jon
>>
>> On 3/29/2013 5:20 PM, Jon Masamitsu wrote:
>>> Ths webrev has mostly name changes suggested by the code
>>> review comments.  There are additional changes coming that
>>> relate to semantic changes suggested by the code review.
>>>
>>> This was created by deleting some of the suggested semantic
>>> changes so will not compile.  The next webrev will be relative
>>> to this one so you won't have to look at the name changes when
>>> looking and the semantic changes.
>>>
>>> Ignore the block comment about chunk accounting at the
>>> beginning of metaspace.cpp.  That will be updated or deleted
>>> in the next webrev.
>>>
>>> The deletion of some dead code is included in this webrev
>>> (limited number of lines).
>>>
>>> http://cr.openjdk.java.net/~jmasa/8008966/webrev.00_1/
>>>
>>> Thanks.
>>>
>>> Jon
>>>
>>> On 3/26/2013 9:13 PM, 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
>>>
>>
>