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

[Richard Reingruber]

On Wed, 30 Sep 2020 13:17:27 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 with a new target base due to a merge or a rebase. The pull request now
> contains 13 commits:
>  - Merge branch 'master' into jep387
>  - Remove accidentally added file
>  - Merge branch 'jep387' of github.com:tstuefe/jdk into jep387
>  - Create submit.yml
>  - Review feedback Ioi
>  - cds MaxMetaspaceSize test does not need MaxMetaspaceSize increase
>  - Make MetaspaceGuardAllocations a diagnostic flag (2)
>  - Make MetaspaceGuardAllocations a diagnostic flag
>  - Style fixes
>  - Remove empty lines from include sections
>  - ... and 3 more: https://git.openjdk.java.net/jdk/compare/f80a6066...f5cf615b

Hi Thomas,

I've viewed 41/166 files. I'm sending you my findings so far so we can work concurrently. Also I'm curious how it goes
sending a partial review and then continuing.

Probably I won't manage to review all the tests but I do hope I get to read all files of the implementation. Working on
it now...

Btw: I couldn't find out how to navigate from one of my comments to the line of code. At least, when I click on the
file name, this brings me to a view of the patch at the version I commented on so the line numbers are correct even if
they changed in the head revision of the pr.

Cheers, Richard.

src/hotspot/share/services/virtualMemoryTracker.cpp line 679:

> 677: }
> 678:
> 679: void MetaspaceSnapshot::snapshot(MetaspaceSnapshot& mss) {

This is not part of your change, but aren't `ClassType` and `NonClassType` mixed up in the body of
`MetaspaceSnapshot::snapshot` method?

src/hotspot/share/memory/metaspace.cpp line 482:

> 480: bool Metaspace::initialized() {
> 481:   return metaspace::MetaspaceContext::context_nonclass() != NULL &&
> 482:       (using_class_space() ? metaspace::MetaspaceContext::context_class() != NULL : true);

Better delegate to `Metaspace::class_space_is_initialized` here?

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

Changes requested by rrich (Committer).

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