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

[Richard Reingruber]

On Fri, 2 Oct 2020 06:39:26 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 two additional commits since the last revision:
> 
>  - Misc. Review feedback
>  - Move ClassLoaderMetaspace to own include and correct includes

src/hotspot/share/memory/classLoaderMetaspace.cpp line 90:

> 88:   }
> 89:
> 90:   UL2(debug, "born (SpcMgr nonclass: " PTR_FORMAT ", SpcMgr class: " PTR_FORMAT ".",

I'd suggest  to replace 'SpcMgr [non]class' with '[non]class arena' or similar since SpaceManager is removed with the
change.

src/hotspot/share/memory/classLoaderMetaspace.cpp line 58:

> 56:
> 57: static bool use_class_space(Metaspace::MetadataType mdType) {
> 58:   return use_class_space(Metaspace::is_class_space_allocation(mdType));

Calling `use_class_space()` is not needed. `Metaspace::is_class_space_allocation()` already queries
`Metaspace::using_class_space()`

src/hotspot/share/memory/classLoaderMetaspace.cpp line 55:

> 53:   }
> 54:   return false;
> 55: }

The following would be equivalent and a lot shorter

static bool use_class_space(bool is_class) {
  return Metaspace::using_class_space() && is_class;
}

src/hotspot/share/memory/classLoaderMetaspace.cpp line 160:

> 158:   DEBUG_ONLY(InternalStats::inc_num_deallocs();)
> 159:
> 160: }

Too many empty lines if you're asking me ;)

src/hotspot/share/memory/metaspace.hpp line 136:

> 134:
> 135:   static void report_metadata_oome(ClassLoaderData* loader_data, size_t word_size,
> 136:                                    MetaspaceObj::Type type, Metaspace::MetadataType mdtype, TRAPS);

The change is not necessary.
You havn't changed the definition either (metaspace.cpp:830, link does not seem to work).
https://github.com/openjdk/jdk/pull/336/files#diff-07bf2be25d84c251ed6107aab47fd009R830

src/hotspot/share/memory/metaspace/allocationGuard.hpp line 68:

> 66: //  -XX:+MetaspaceGuardAllocations. When active, it disables deallocation handling - since
> 67: //  freeblock handling in the freeblock lists would get too complex - so one may run leaks
> 68: //  in deallocation-heavvy scenarios (e.g. lots of class redefinitions).

Typo: heavvy

src/hotspot/share/memory/metaspace/binList.hpp line 141:

> 139:       assert(_blocks[i2] != NULL, "mask mismatch");
> 140:       return i2;
> 141:     }

There would be `count_leading_zeros()`.
If I'm not mistaken, it is _trailing_ zeros that are counted. So you could use
[count_trailing_zeros()](https://github.com/openjdk/jdk/blob/d296708ca66ab15e31a05f963a5e162a3e3f07f9/src/hotspot/share/utilities/count_trailing_zeros.hpp#L31)

src/hotspot/share/memory/metaspace/blockTree.hpp line 219:

> 217:
> 218:   // Given a node n and an insertion point, insert n under insertion point.
> 219:   void insert(Node* insertion_point, Node* n) {

Could be static

src/hotspot/share/memory/metaspace/blockTree.hpp line 193:

> 191:         n2 = succ;
> 192:         succ = succ->_parent;
> 193:       }

You could inline just the then-block into `remove_node_from_tree()` which is the only caller of `successor()`

src/hotspot/share/memory/metaspace/blockTree.hpp line 174:

> 172:     return pred;
> 173:   }
> 174:

Looks like `predecessor()` is not used.

src/hotspot/share/memory/metaspace/freeChunkList.hpp line 74:

> 72: //  structure can easily be optimized (e.g. a BST). But for now lets keep this simple.
> 73:
> 74: class FreeChunkList {

This is about the 3rd list implementation (so far) in this change (boilerplate code!). It would be awesome if one or
two could be replaced with
[LinkedList](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/utilities/linkedlist.hpp). Currently this is
not possible, because `LinkedListNodes` cannot be placed into the payload. What about filing a RFE about adding and
using a `LinkedList` implementation that allows this?

src/hotspot/share/memory/metaspace/chunkManager.hpp line 118:

> 116:
> 117:   // On success, returns a chunk of level of <preferred_level>, but at most <max_level>.
> 118:   //  The first first <min_committed_words> of the chunk are guaranteed to be committed.

Typo: first first.

src/hotspot/share/memory/metaspace/chunkManager.cpp line 55:

> 53:   DEBUG_ONLY(c->verify());
> 54:
> 55:   const chunklevel_t lvl = c->level();

lvl is unused.

src/hotspot/share/memory/metaspace/chunkManager.cpp line 144:

> 142:   DEBUG_ONLY(chunklevel::check_valid_level(max_level);)
> 143:   DEBUG_ONLY(chunklevel::check_valid_level(preferred_level);)
> 144:   assert(max_level >= preferred_level, "invalid level.");

This assert is redundant. It has the same condition as the assert on line 135.

src/hotspot/share/memory/metaspace/rootChunkArea.cpp line 89:

> 87:   //    created chunk pair)
> 88:   // - then create a new chunk header of the same level, set its memory range
> 89:   //   to cover the second halfof the old chunk.

Typo: halfof

src/hotspot/share/memory/metaspace/rootChunkArea.cpp line 110:

> 108:   // half chunk splinter.
> 109:   // Instead, just split split split and delay building up the double linked list of the
> 110:   // new chunks at the end of all splits.

This comment does not match the code below anymore, does it?

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

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