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

[Thomas Stüfe]

Thanks, Coleen!

Sorry, I forgot to wait for your acknowledgment. I got confused, too many
patches for review.

..Thomas

On Tue 24. Apr 2018 at 18:56, <coleen.phillimore at oracle.com> wrote:

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