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

[Siebenborn, Axel]

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
> >>
> >>