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

[Thomas Stüfe]

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