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

[Coleen Phillimore]

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

Besides my comment about globals.hpp option MetaspaceGuardAllocations, my comments are minor things and I approve this
change.  There might be some additional things we find that we'll want to change once this code is integrated.  This is
a significant improvement to metaspace memory management.  Great work, @tstuefe !

src/hotspot/share/memory/metaspace/blockTree.cpp line 72:

> 70: };
> 71:
> 72: void BlockTree::verify() const {

I think this sort of deep verification should be in a gtest instead, at least for blockTree.  Note, this can be
fixed/discussed in a future RFE.

src/hotspot/share/memory/metaspace/chunkHeaderPool.hpp line 107:

> 105:     return c;
> 106:
> 107:   }

I don't usually comment on style, but there are so many blank lines in this function.

src/hotspot/share/memory/metaspace/classLoaderMetaspace.cpp line 64:

> 62:   , _space_type(space_type)
> 63:   , _non_class_space_arena(NULL)
> 64:   , _class_space_arena(NULL)

Ok, I see what @lkorinth was commenting on.  In the coding standard, the normal indentation level is 2, but we don't
specify it for initializers.  Generally it seems what looks good, maybe 4, maybe aligned somewhat with the arguments.
That said to me it doesn't matter that much, BUT most of the code has the punctuation at the end of the line, not the
beginning.  I think it looks weird at the beginning.  Can you change these?

src/hotspot/share/memory/metaspace/commitMask.hpp line 92:

> 90:     check_pointer(start + word_size - 1);
> 91:   }
> 92: #endif

If these are for asserts, they can be defined in the .cpp file.

src/hotspot/share/memory/metaspace/metachunk.hpp line 272:

> 270:     _vsnode = node; _base = base; _level = lvl;
> 271:     _used_words = _committed_words = 0; _state = State::Free;
> 272:     _next = _prev = _next_in_vs = _prev_in_vs = NULL;

Is this the same as clear() ?

src/hotspot/share/memory/metaspace/metachunkList.hpp line 60:

> 58:   void add(Metachunk* c) {
> 59:     // Note: contains is expensive (linear search).
> 60:     ASSERT_SOMETIMES(contains(c) == false, "Chunk already in this list");

Can you make this something like:  DEBUG_ONLY(verify_contains();) and hide ASSERT_SOMETIMES in the .cpp file?

src/hotspot/share/memory/metaspace/metachunkList.hpp line 29:

> 27: #define SHARE_MEMORY_METASPACE_METACHUNKLIST_HPP
> 28:
> 29: #include "memory/metaspace/counters.hpp"

Is this header file needed now?

src/hotspot/share/runtime/globals.hpp line 1589:

> 1587:                                                                             \
> 1588:   product(bool, MetaspaceGuardAllocations, false,                           \
> 1589:           "Metapace allocations are guarded.")                              \

This should be DIAGNOSTIC or develop() but not product.

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

Marked as reviewed by coleenp (Reviewer).

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