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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Tue Apr 24 16:56:20 UTC 2018



On 4/21/18 2:39 AM, Thomas Stüfe wrote:
> Looking closely, I am quite convinced that the boot metaspace chunk is
> allocated when the ClassLoaderMetaspace is created, via:
>
> ClassLoaderMetaspace::initialize()->ClassLoaderMetaspace::initialize_first_chunk()
>
> 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?

Yes, it's fine (I know you pushed but just to wrap this up).
Thanks,
Coleen
>
> --
>
> 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
> mismatch.
>
> 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