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

[coleen.phillimore at oracle.com]

Thomas,

This looks like a good cleanup.   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?

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