RFR: 8251158: Implementation of JEP 387: Elastic Metaspace

[Stefan Karlsson]

On Thu, 24 Sep 2020 12:16:55 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

src/hotspot/share/memory/metaspace/virtualSpaceNode.cpp line 47:

> 45: #include "runtime/mutexLocker.hpp"
> 46: #include "runtime/os.hpp"
> 47:

Just a drive-by comment. Please don't introduce this new style of adding newlines between our internal include lines.
The same comment about the newlines after precompiled.hpp. I know that the Shenandoah code already does that, and I've
seen some of the metaspace code do that as well, but please stick to the set standard way of listing our includes. This
applies to all files in this patch.

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

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