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

[Leo Korinth]

On Fri, 25 Sep 2020 11:00:20 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:
> 
>   Remove empty lines from include sections

I just have a few cosmetic comments. Otherwise it looks good to me.

If you sort by ASCII order, I get a few unordered includes:
("." < "/" < "a-z,A-Z")


In file b/src/hotspot/share/gc/shared/genCollectedHeap.cpp:
[not sorted]:
#include "memory/metaspaceCounters.hpp" (Context)
#include "memory/metaspace/metaspaceSizesSnapshot.hpp" (Addition)
#include "memory/resourceArea.hpp" (Context)

In file b/src/hotspot/share/memory/metaspace.cpp:
[not sorted]:
#include "memory/metaspace/virtualSpaceList.hpp" (Context)
#include "memory/metaspace.hpp" (Addition)
#include "memory/metaspaceShared.hpp" (Context)

In file b/src/hotspot/share/memory/metaspace/chunkManager.cpp:
[not sorted]:
#include "memory/metaspace/metaspaceCommon.hpp" (Context)
#include "memory/metaspace/metaspaceContext.hpp" (Addition)
#include "memory/metaspace/metachunk.hpp" (Addition)

In file b/src/hotspot/share/memory/metaspace/chunkManager.cpp:
[not sorted]:
#include "memory/metaspace/metaspaceContext.hpp" (Addition)
#include "memory/metaspace/metachunk.hpp" (Addition)
#include "memory/metaspace/metaspaceSettings.hpp" (Addition)


blockTree.hpp
add a space after loop keyword "for(;;) {" -> "for (;;) {"

blockTree.cpp
add a space after loop keyword "} while(0)" -> "} while (0)" (twice in file)

I still think we should try to get the initializer list indented
somewhat consistently. (This is really boring, and hard as we do not
have precise indentation rules and no mechanical indenter). Sorry for
mentioning this, but now might be the time to get indentation
consistent at least in metaspace. The indentation level seems to most
often be 2 or 4. Sometimes "Haskell" indentation with "," at the
beginning of each line is used (I like this, but I think I am in a
small minority) and most often it is not used. Sometimes several field
members are initialised on multi line initializer lists, sometimes
only one per line (I prefer one per line, if not all fits on a single
line). Open curly braces seem to mostly (but not always) be on a new line.

If you could normalize the style I would be happy, I yield my own
preferences to you and the other reviewers on what style to use, so
that we have a possibility to agree :-)

Thanks, Leo

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

Changes requested by lkorinth (Committer).

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