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

Tue Apr 24 09:59:58 UTC 2018

Thank you Axel!

..Thomas

On Tue, Apr 24, 2018 at 9:33 AM, Siebenborn, Axel
<axel.siebenborn at sap.com> wrote:
> Hi Thomas,
>
> the change looks good to me.
> Nice cleanup and makes the code much easier to understand.
>
> Cheers,
> Axel
>
>> -----Original Message-----
>> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
>> bounces at openjdk.java.net] On Behalf Of Thomas Stüfe
>> Sent: Samstag, 21. April 2018 08:40
>> To: Coleen Phillmore <coleen.phillimore at oracle.com>
>> Cc: Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net>
>> Subject: Re: RFR(xs): 8202074: Metaspace: If humongous chunk is added to
>> SpaceManager, previous current chunk may not get retired correctly.
>>
>> 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?
>>
>> --
>>
>> 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.u
>> diff.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
>> >>
>> >>