RFR(xs): 8202074: Metaspace: If humongous chunk is added to SpaceManager, previous current chunk may not get retired correctly.

Thomas Stüfe thomas.stuefe at gmail.com
Sat Apr 21 06:39:44 UTC 2018

Looking closely, I am quite convinced that the boot metaspace chunk is
allocated when the ClassLoaderMetaspace is created, via:


where we first call ClassLoaderMetaspace::get_initialization_chunk(),
then add it via SpaceManager::add_chunk(chunk, true).

So, we never go thru SpaceManager::grow_and_allocate(), and my comment
would never apply.

Thinking this further, I am not sure we ever call
SpaceManager::grow_and_allocate() with current_chunk() == NULL, since
the first chunk of each metaspace seems always to be created via the
above mentioned path.

But still, for now I'd like to keep the comment as it is. Do you agree?


Side note: This
allocate-the-initial-chunk-at-classloaderdata-creation-time business
is a bit annoying. I found it complicates statistic keeping, since at
that time the ClassLoaderData is not yet put into the CLDG - so any
outside statistic walking the CLDG and counting will not see this
metaspace yet. But since it already did create a chunk, it already
modifies the global counters in MetaspaceUtils (former MetaspaceAux).
So, in that time window we cannot verify the counters since we get a

Basically, global counters include allocations from CLD not yet in
CLDG, but statistics walking the CLDG exclude them, giving us
different numbers..

So I wonder why we even do this. Why not allocate an empty metaspace
and init the first chunk lazily when the first allocation happens.
Something like this, in SpaceManager::grow_and_allocate():

if (current_chunk == NULL) // first chunk
chunk_size = get_initialization_chunk_size();

and then proceed like we do for any other chunk...


But all this is out of scope for this item.

Kind Regards, Thomas

On Fri, Apr 20, 2018 at 4:19 PM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
> Hi Coleen,
> On Fri, Apr 20, 2018 at 2:36 PM,  <coleen.phillimore at oracle.com> wrote:
>> Thomas,
>> This looks like a good cleanup.
> Thank you.
>   Is the boot metaspace chunk a humongous
>> chunk, but it is made current by the check for current_chunk() != NULL
>> above, right?   Can you say that in the comment too?
> I am actually not sure the boot metaspace chunk is allocated here. Is
> it not rather allocated
> at ClassLoaderMetaspace::initialize_first_chunk()? (I'm away from my
> machine and cannot check it).
> ..Thomas
>> + // If the new chunk is humongous, it was created to serve a single large
>> allocation. In that
>> + // case it usually makes no sense to make it the current chunk, since the
>> next allocation would
>> + // need to allocate a new chunk anyway, while we would now prematurely
>> retire a perfectly
>> + // good chunk which could be used for more normal allocations.
>> + bool make_current_chunk = true;
>> + if (next->get_chunk_type() == HumongousIndex &&
>> + current_chunk() != NULL) {    <= null class loader data and
>> DumpSharedSpaces, true?
>> + make_current_chunk = false;
>> + }
>> Thanks,
>> Coleen
>> On 4/20/18 3:53 AM, Thomas Stüfe wrote:
>>> Hi all,
>>> may I have reviews for this fix please:
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8202074
>>> webrev:
>>> http://cr.openjdk.java.net/~stuefe/webrevs/8202074-metaspace-retire-after-humongous-chunks-are-allocated/webrev.00/webrev/src/hotspot/share/memory/metaspace.cpp.udiff.html
>>> When a humongous chunk is added to SpaceManager
>>> (SpaceManager::add_chunk(chunk, make_current=true), we make it the
>>> current chunk.
>>> But we do not correctly retire the old chunk. This leads to the
>>> remainder of the used space in the old chunk being wasted.
>>> The obvious smallest-possible fix would be this:
>>> --- a/src/hotspot/share/memory/metaspace.cpp Mon Apr 16 14:29:27 2018
>>> +0530
>>> +++ b/src/hotspot/share/memory/metaspace.cpp Fri Apr 20 06:55:04 2018
>>> +0200
>>> @@ -3573,6 +3573,7 @@
>>>       // chunk.
>>>       if (make_current) {
>>>         // Set as the current chunk but otherwise treat as a humongous
>>> chunk.
>>> +      retire_current_chunk();
>>>         set_current_chunk(new_chunk);
>>>       }
>>>       // Link at head.  The _current_chunk only points to a humongous
>>> chunk for
>>> However, I decided to clean the code up a bit and make it more
>>> readable. In SpaceManager::add_chunk(), treatment of the non-humongous
>>> chunks and humongous chunks is almost identical, beside the fact that
>>> non-humongous new chunks were always made current and the argument
>>> flag "make_current" was only honored for humongous chunks.
>>> I changed that. I simplified SpaceManager::add_chunk() by removing the
>>> extra treatment for humongous chunks, instead moving the humongous
>>> chunk related decision up to SpaceManager::grow_and_allocate(),
>>> together with a clear comment. That has the side effect that
>>> SpaceManager::add_chunk(chunk, make_current) now always honors the
>>> make_current flag.
>>> Thank you, Thomas

More information about the hotspot-runtime-dev mailing list