RFR: 8251158: Implementation of JEP 387: Elastic Metaspace [v4]

[Thomas Stuefe]

On Wed, 30 Sep 2020 00:48:21 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Make MetaspaceGuardAllocations a diagnostic flag (2)
>
> test/hotspot/jtreg/runtime/cds/MaxMetaspaceSize.java line 47:
> 
>> 45:
>> 46:     if (Platform.is64bit()) {
>> 47:       processArgs.add("-XX:MaxMetaspaceSize=8m");
> 
> Does this mean the absolute minimal size is larger than before? I just want to confirm this. I think 3M -> 8M doesn't
> really matter, unless other (larger) minimums also scale up by a factor of 2.66 :-)

On the contrary. Minimum metaspace usage to run a simple HelloWorld went way down.

Before: 10,00 MB reserved,       4,75 MB ( 48%) committed
Now: 12,00 MB reserved,     448,00 KB (  3%) committed.

(CDS enabled in both cases)

mainly because now we commit the large chunk for the boot loader only on demand. Reservation (vsize) numbers are
somwhat chunkier now since the default size for a VirtualSpaceNode went up from 4 to 8MB.

You now can get away with -XX:MaxMetaspaceSize=~500K to start a simple program.

While writing this, I wondered why I made this change to your test. It is not needed for the current version, probably
a remnant from some earlier prototype. I will revert this line and check the other tests too.

> src/hotspot/share/gc/parallel/psParallelCompact.cpp line 1061:
> 
>> 1059:   // Delete metaspaces for unloaded class loaders and clean up loader_data graph
>> 1060:   ClassLoaderDataGraph::purge(/*at_safepoint*/true);
>> 1061:   DEBUG_ONLY(MetaspaceUtils::verify();)
> 
> I think it will be cleaner to declare MetaspaceUtils::verify() as
> 
> void verify() NOT_DEBUG_RETURN;
> 
> then you can omit the `DEBUG_ONLY` at every caller.

Unless you really want me to, or other Reviewers chime in, I'd rather leave it as it is. I prefer this style since it
is clear at the callsites that this is only ASSERT code.

> src/hotspot/share/runtime/globals.hpp line 1584:
> 
>> 1582:   product(uintx, ForceCompressedClassSpaceStartAddress, 0, EXPERIMENTAL,    \
>> 1583:           "Force class space start address to a given value.")              \
>> 1584:                                                                             \
> 
> ForceCompressedClassSpaceStartAddress doesn't seem to be used

Good catch, will remove.

> src/hotspot/share/memory/metaspace.hpp line 164:
> 
>> 162: //
>> 163: // ClassLoaderMetaspace only exists to hide this logic from upper layers:
>> 164: //
> 
> I would suggest rewriting the comments to
> 
> // A ClassLoaderMetaspace manages  MetaspaceArena(s) for a CLD.
> //
> // A CLD owns one MetaspaceArena if UseCompressedClassPointers is false. Otherwise
> // it owns two -- one for the Klass* objects from the class space, one for the other types
> // of MetaspaceObjs from the non-class space.
> 
> (I think "hide this logic ...." can be omitted. We have lots of abstractions so there's no need to explicitly call it
> out).

Okay, sounds good.

-------------

PR: https://git.openjdk.java.net/jdk/pull/336