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

Wed Sep 30 01:06:51 UTC 2020

On Tue, 29 Sep 2020 07:58:06 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Hi all,
>> 
>> this is the continuation of the ongoing review for the JEP387 implementation (last rounds see [1] [2]). Sorry for the
>> delay, had vacation then the entrance of Skara delayed things a bit.
>> For the delta diff please see [3].
>> 
>> This is the first time I do a large PR after Skara, so if something is wrong please bear with me. I cannot answer all
>> feedback individually in this PR body, but I incorporated almost all into the new revision.
>> What changed since the last version:
>> 
>> - I renamed most metaspace files back to the original naming scheme or to something similar, hopefully capturing the
>>   group consent.
>> 
>> - I changed the way allocation guards are checked if MetaspaceGuardAllocations is enabled. Before, I would test for
>>   overwrites upon CLD destruction, but since that check was subject to VerifyMetaspaceInterval it only ran for every nth
>>   class loader which made it rather pointless. Now I run it always.
>> 
>> - I also improved the printout on block corruption, and log block corruption unconditionally before asserting.
>> 
>> - I also fixed up and commented the death test which tests for allocation overwriters (test_allocationGuard.cpp)
>> 
>> Side note, I find the corruption check very useful but if you guys think it is too much I still can remove the feature.
>> 
>> - In ChunkManager::purge() I improved the comments after discussions with Leo.
>> 
>> - I fixed a bug with VerifyMetaspaceInterval: if set to 1 the "SOMETIMES" sections were supposed to fire always, but due
>>   to a one-off error they only fired every second time. Now, if -XX:VerifyMetaspaceInterval=1, the checks really run
>>   every time.
>> 
>> - Fixed indentation issues as Leo requested
>> 
>> - Rewrote the condition and the assert in VirtualSpaceList::allocate_root_chunk() as Leo requested
>> 
>> - I removed the "can_purge" logic from VirtualSpaceList. The list does not need to know. It just should iterate all nodes
>>   and attempt purging, and if a node does not own its ReservedSpace, it refuses to be purged. That is simpler and more
>>   flexible since it allows us to have list with purge-able and non-purge-able nodes.
>> 
>> - and various smaller fixes, mainly on request of Leo.
>> 
>> @lkorinth:
>> 
>>> VirtualSpaceNode.hpp
>>>
>>>102   // Start pointer of the area.
>>>103   MetaWord* const _base;
>>>
>>>How does this differ from _rs._base? Really needed?
>>>
>>>105   // Size, in words, of the whole node
>>>106   const size_t _word_size;
>>>
>>>Can we not calculate this from _rs.size()?
>> 
>> You are right, _base and _word_size are directly related to the underlying space. But I'd prefer to leave it the way it
>> is. Mainly because ReservedSpace::_base and ::_size are nonconst and theoretically can change under me. It is highly
>> improbable but I'd like to know. Note that VirtualSpaceNode::verify checks that.  Should we clean up ReservedSpace at
>> some point and make those members const - as they should be - then I would rewrite this as you suggest.
>> Thanks, again, for all your review work!
>> 
>> ------
>> 
>> 
>> [1] https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-August/041162.html
>> [2] https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-September/041628.html
>> [3] https://github.com/openjdk/jdk/commit/731f795bc0c1c502dc6cac8f866ff45a15bdd02d
>
> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Make MetaspaceGuardAllocations a diagnostic flag (2)

So far I have only reviewed the C code that uses the metaspace, but not the metaspace implementation itself
(memory/metaspace*, memory/metaspace/*). I also reviewed a bit of classes.

src/hotspot/share/jvmci/jvmciCompilerToVM.hpp line 28:

> 26:
> 27: #include "gc/shared/cardTable.hpp"
> 28: #include "gc/shared/collectedHeap.hpp"

This should be changed to a forward declaration to reduce the number of headers included by headers:
`class CollectedHeap;`

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

src/hotspot/share/services/memReporter.cpp line 26:

> 24: #include "precompiled.hpp"
> 25:
> 26:

Unneeded new line

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.

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

test/hotspot/jtreg/runtime/cds/appcds/sharedStrings/LargePages.java line 49:

> 47:         SharedStringsUtils.dump(TestCommon.list("HelloString"),
> 48:             "SharedStringsBasic.txt", CDS_LOGGING,
> 49:             "-XX:+UseLargePages");

You can delete code from line 47 onwards as they are the same as the  test cases above.

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

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

Changes requested by iklam (Reviewer).

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