RFR: 8251158: Implementation of JEP 387: Elastic Metaspace

Reingruber, Richard richard.reingruber at sap.com
Mon Aug 31 16:05:40 UTC 2020


Hi Thomas,

the goals of jep387 do make sense and yet even better: the implementation lives up to
them as you showed with tests and benchmarks. Thanks for putting all the effort
into it!

Sorry for being such a slow reviewer. I haven't even completed a full pass yet
but I think it is time to share my findings so far.

Thanks, Richard.

======

You added includes of memory/metaspace.hpp to some files without additional
modifications. How did you chose these files?

To jvmciCompilerToVM.hpp you added an include of collectedHeap.hpp. Why did you
do this?

====== src/hotspot/share/memory/metaspace.cpp

 194       // ... and class spacelist.
 195       VirtualSpaceList* vsl = VirtualSpaceList::vslist_nonclass();
 196       assert(vsl != NULL, "Sanity");
 197       vsl->verify(slow);

In 195 VirtualSpaceList::vslist_class() should be called I suppose.

You could reuse the local vsl as you did with the local cm.
Any reason you assert vsl not NULL in 196 but not in the non-class case?

 637     // CCS must be aligned to root chunk size, and be at least the size of one
 638     //  root chunk.
 639     adjusted_ccs_size = align_up(adjusted_ccs_size, reserve_alignment());
 640     adjusted_ccs_size = MAX2(adjusted_ccs_size, reserve_alignment());

Line 640 is redundant, isn't it?

@@ -1274,7 +798,7 @@
   assert(loader_data != NULL, "Should never pass around a NULL loader_data. "
         "ClassLoaderData::the_null_class_loader_data() should have been used.");
 
-  MetadataType mdtype = (type == MetaspaceObj::ClassType) ? ClassType : NonClassType;
+  Metaspace::MetadataType mdtype = (type == MetaspaceObj::ClassType) ? Metaspace::ClassType : Metaspace::NonClassType;
 
   // Try to allocate metadata.
   MetaWord* result = loader_data->metaspace_non_null()->allocate(word_size, mdtype);

This hunk is not needed.

====== src/hotspot/share/memory/metaspace/binlist.hpp

  94	  // block sizes this structure can keep are limited by [_min_block_size, _max_block_size)
  95	  const static size_t minimal_word_size = smallest_size;
  96	  const static size_t maximal_word_size = minimal_word_size + num_lists;

_min_block_size/_max_block_size should be minimal_word_size/maximal_word_size.

The upper limit 'maximal_word_size' should be inclusive IMHO:

  const static size_t maximal_word_size = minimal_word_size + num_lists - 1;

That would better match the meaning of the variable name. Checks in l.148 and
l.162 should be adapted in case you agree.

 181	
 182	    } else {
 183	
 184	      *p_real_word_size = 0;
 185	      return NULL;
 186	
 187	    }

Lines 181, 183, 186 can be deleted.

====== src/hotspot/share/memory/metaspace/blocktree.hpp

The file would benefit from a whitespace clean-up.

  43	// We store node pointer information in these blocks when storing them. That
  44	//  imposes a minimum size to the managed memory blocks.
  45	//  See MetaspaceArene::get_raw_allocation_word_size().

s/MetaspaceArene::get_raw_allocation_word_size/metaspace::get_raw_word_size_for_requested_word_size/

I agree with the comment, but
metaspace::get_raw_word_size_for_requested_word_size() does not seem to take
this into account.

  86	    // blocks with the same size are put in a list with this node as head.
  89	    // word size of node. Note that size cannot be larger than max metaspace size,
 115	  // given a node n, add it to the list starting at head
 123	  // given a node list starting at head, remove one node from it and return it.

You should begin a sentence consistently with a capital letter (you mostly do it).

 123	  // given a node list starting at head, remove one node from it and return it.
 124	  // List must contain at least one other node.
 125	  static node_t* remove_from_list(node_t* head) {
 126	    assert(head->next != NULL, "sanity");
 127	    node_t* n = head->next;
 128	    if (n != NULL) {
 129	      head->next = n->next;
 130	    }
 131	    return n;
 132	  }

Line 129 must be executed unconditionally.

I'd prefer a more generic implementation that allows head->next to be
NULL. Maybe even head == NULL.

 134	  // Given a node n and a node p, wire up n as left child of p.
 135	  static void set_left_child(node_t* p, node_t* c) {
 143	  // Given a node n and a node p, wire up n as right child of p.
 144	  static void set_right_child(node_t* p, node_t* c) {

Comments should use same variable names as method definition.

 215	  // Given a node n and a node forebear, insert n under forebear
 216	  void insert(node_t* forebear, node_t* n) {
 217	    if (n->size == forebear->size) {
 218	      add_to_list(n, forebear); // parent stays NULL in this case.
 219	    } else {
 220	      if (n->size < forebear->size) {
 221	        if (forebear->left == NULL) {
 222	          set_left_child(forebear, n);
 223	        } else {
 224	          insert(forebear->left, n);
 225	        }
 226	      } else {
 227	        assert(n->size > forebear->size, "sanity");
 228	        if (forebear->right == NULL) {
 229	          set_right_child(forebear, n);
 230	          if (_largest_size_added < n->size) {
 231	            _largest_size_added = n->size;
 232	          }
 233	        } else {
 234	          insert(forebear->right, n);
 235	        }
 236	      }
 237	    }
 238	  }

This assertion in line 227 is redundant (cannot fail).

  Leo> There are at least two recursive calls of insert that could be
  Leo> tail-called instead (it would be somewhat harder to read, so I am not
  Leo> proposing it).

I think they _are_ tail-recursions in the current form.
Gcc eliminates them. I checked the release build with gdb:
(disass /s metaspace::FreeBlocks::add_block)

Recursive tail-calls can be easily replaced with loops. To be save I'd suggest
to do that or at least add 'return' after each call with a comment that nothing
must be added between the call and the return too keep it a
tail-recursion. Maybe that's sufficient... on the other hand we don't know if
every C++ compiler can eliminate the calls and stack overflows when debugging
would be also irritating.

 251	        return find_closest_fit(n->right, s);
 260	        return find_closest_fit(n->left, s);

More tail-recursion. Same as above.

 257	      assert(n->size > s, "Sanity");

Assertion is redundant.

 262	        // n is the best fit.
 263	        return n;

In the following example it is not, is it?

          N1:40
           /
          /
       N2:20
          \
           \
         N3:30

find_closest_fit(N1, 30) will return N2 but N3 is the closest fit. I think you
have to search the left tree for a better fit independently of the size of its
root node.

 293	    if (n->left == NULL && n->right == NULL) {
 294	      replace_node_in_parent(n, NULL);
 295	
 296	    } else if (n->left == NULL && n->right != NULL) {
 297	      replace_node_in_parent(n, n->right);
 298	
 299	    } else if (n->left != NULL && n->right == NULL) {
 300	      replace_node_in_parent(n, n->left);
 301	
 302	    } else {

Can be simplified to:

    if (n->left == NULL) {
      replace_node_in_parent(n, n->right);
    } else if (n->right == NULL) {
      replace_node_in_parent(n, n->left);
    } else {

 341	        // The right child of the successor (if there was one) replaces the successor at its parent's left child.

Please add a line break.

The comments and assertions in remove_node_from_tree() helped to understand the
logic. Thanks!

====== src/hotspot/share/memory/metaspace/blocktree.cpp

  40	// These asserts prints the tree, then asserts
  41	#define assrt(cond, format, ...) \
  42	  if (!(cond)) { \
  43	    print_tree(tty); \
  44	    assert(cond, format, __VA_ARGS__); \
  45	  }
  46	
  47	  // This assert prints the tree, then stops (generic message)
  48	#define assrt0(cond) \
  49		  if (!(cond)) { \
  50		    print_tree(tty); \
  51		    assert(cond, "sanity"); \
  52		  }

Better wrap into do-while(0) (see definition of vmassert)

  66	  while (n2 != NULL) {
  67	    assrt0(n2->size == size);
  68	    vd->counter.add(n2->size);
  69	    if (prev_sib != NULL) {
  70	      assrt0(prev_sib->next == n2);
  71	      assrt0(prev_sib != n2);
  72	    }
  73	    prev_sib = n2;
  74	    n2 = n2->next;
  75	  }

The assertion in line 70 cannot fail.

 110	    verify_node(n->left, left_limit, n->size, vd, lvl + 1);

Recursive call that isn't a tail call. Prone to stack overflow. Well I guess you
need a stack to traverse a tree. GrowableArray is a common choice if you want to
eliminate this recursion. As it is only verification code you might as well
leave it and interpret stack overflow as verification failure.

 118	    verify_node(n->right, n->size, right_limit, vd, lvl + 1);

Tail-recursion can be easily eliminated. See comments on blocktree.hpp above.

====== src/hotspot/share/memory/metaspace/chunkManager.cpp

The slow parameter in ChunkManager::verify*() is not used.

====== src/hotspot/share/memory/metaspace/counter.hpp

 104   void decrement() {
 105 #ifdef ASSERT
 106     T old = Atomic::load_acquire(&_c);
 107     assert(old >= 1,
 108         "underflow (" UINT64_FORMAT "-1)", (uint64_t)old);
 109 #endif
 110     Atomic::dec(&_c);
 111   }

I think you could use Atomic::add() which returns the old value and make the assert atomic too:

  void decrement() {
    T old = Atomic::add(&_c, T(-1));
#ifdef ASSERT
    assert(old >= 1,
           "underflow (" UINT64_FORMAT "-1)", (uint64_t)old);
#endif
  }

Same for increment(), increment_by(), decrement_by(), ...

====== src/hotspot/share/memory/metaspace/metaspaceArena.cpp

There's too much vertical white space, I'd think.

metaspace::get_raw_allocation_word_size() is a duplicate of
metaspace::get_raw_word_size_for_requested_word_size()
metaspace::get_raw_allocation_word_size() is only referenced in comments and
should be removed.

====== src/hotspot/share/memory/metaspace/metaspaceCommon.cpp

 174 // Given a net allocation word size, return the raw word size we actually allocate.
 175 // Note: externally visible for gtests.
 176 //static
 177 size_t get_raw_word_size_for_requested_word_size(size_t word_size) {

There are call sites in other compilation units too. The lines 175, 176 can be
removed I'd say.

 181	  // Deallocated metablocks are kept in a binlist which limits their minimal
 182	  //  size to at least the size of a binlist item (2 words).
 183	  byte_size = MAX2(byte_size, FreeBlocks::minimal_word_size * BytesPerWord);

byte_size should also depend on BlockTree::minimal_word_size I think. Something like

if (worde_size > FreeBlocks::maximal_word_size)
  byte_size = MAX2(byte_size, BlockTree::minimal_word_size * BytesPerWord);

FreeBlocks::maximal_word_size needs to be defined for this.

-----Original Message-----
From: hotspot-gc-dev <hotspot-gc-dev-retn at openjdk.java.net> On Behalf Of Thomas Stüfe
Sent: Mittwoch, 12. August 2020 20:00
To: Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net>; Hotspot-Gc-Dev <hotspot-gc-dev at openjdk.java.net>
Subject: RFR: 8251158: Implementation of JEP 387: Elastic Metaspace

Dear all,

I would like to start the review for the implementation of JEP 387 "Elastic
Metaspace".

The relevant JBS items are:

JEP: https://openjdk.java.net/jeps/387

Implementation Issue (pretty much only a placeholder currently):
https://bugs.openjdk.java.net/browse/JDK-8251158

--

Reminder of why we do this:

1. The new metaspace saves memory. How much depends on context: if it is
not a problem today it will continue to be none. But in cases where today
we face large metaspace consumption we may get reductions, sometimes
drastic ones. These reductions are caused by two facts:
- after mass class unloading we release memory more promptly to the OS
- even without class unloading chunk allocation is just plain smarter,
which reduces the overhead per class loader. This is especially true for
scenarios involving masses of small loaders which only load very few
classes.

As an example, see [1] where two VMs - one stock, one patched - run the
same test program which creates tons of small loaders. The second run shows
a much reduced memory footprint and increased elasticity.

2. The rewritten metaspace code base got cleaner and less complex and thus
should be much easier to maintain and to extend. It also would be possible
to easily reuse the allocator for different parts of the VM, since it is
less tightly tied to the notion of just storing class metadata.

--

In preparation of this review I prepared a guide [2], [3], which gives an
architectural overview and should be the starting point for an interested
Reviewer.

The prototype has been tested extensively for quite some time in SAP's CI
system. We regularly run JCK test, JTReg tests and a whole battery of SAP
internal tests on 8 platforms. Tests are also currently ongoing at Oracle
and Red Hat.

--

The full webrev [4] is somewhat large. In order to make this more bearable
I broke the patch up into three parts:

1) The core parts [5]

This is the heart of the metaspace, mostly rewritten from scratch. I
propose any Reviewer should not look at the diff but rather just examine
the new implementation. One possible exception is metaspace.hpp, which is
the outside interface to metaspace.

There are several ways to get a feeling for this code (after reading at
least the concept part of the provided guide [2]). The central, most
"beefy" classes are:

- VirtualSpaceNode - does all the work of reserving, committing,
uncommitting memory
- RootChunkArea - does the grunt work of chunk splitting and merging
- ChunkManager - which takes care of the chunk freelists, of directing
chunk splits and merges, of enlarging chunks in place
- MetaspaceArena - which takes care of fine granular allocation on behalf
of a CLD, and of managing deallocated blocks.

One way to review could be bottom up: starting at
VirtualSpaceNode/CommitMask/RootChunkArea(LUT), working your way up to
ChunkManager and Metachunk; finally up to to MetaspaceArena while taking a
side stroll to FreeBlocks/BinList/BlockTree.

Another way would be to follow typical paths:

Allocation path: Starting at MetaspaceArena::allocate() ->
Metachunk::allocate() (note the committing-on-demand code path starting
here) -> ChunkManager::get_chunk() ->
VirtualSpaceList/Node->allocate_root_chunk().

Destruction: ~MetaspaceArena() -> ChunkManager::return_chunk() -> (merging
chunks) -> (uncommitting chunks)

Premature deallocation: -> MetaspaceArena::deallocate() -> see freeblock
list handling

2) The tests [6]

This part of the patch contains all the new tests. There are a lot; the
test coverage of the new metaspace is very thorough.

New gtests have been added under 'test/hotspot/gtest/metaspace'.
New jtreg have been added under
'test/hotspot/jtreg/runtime/Metaspace/elastic' and under
'test/hotspot/jtreg/gtest/MetaspaceGtests.java'.

4) Miscellaneous diffs [7]

This is the part of the patch which is neither considered core nor test.
These changes are small, often uninteresting, and can be reviewed via diff.

---

Out of scope for this patch is revamping outside metaspace statistics:
monitoring of metaspace statistics is mostly left untouched, beyond the
work needed to keep existing tests green. I wanted to keep those changes
separate from the core changes for JEP387. They are tracked in JDK-8251392
[8] and JDK-8251342 [9], respectively.

---

Code complexity:

Numerically, lines of code went ever so slightly down with this patch:

Before: (cloc hotspot/share/memory)
-------------------------------------------------------------------------------
C++ 36 2533 3097 12735
C/C++ Header 54 1728 2867 6278
-------------------------------------------------------------------------------
SUM: 90 4261 5964 19013

After:
-------------------------------------------------------------------------------
C++ 50 3048 3774 13127
C/C++ Header 63 2006 3575 5738
-------------------------------------------------------------------------------
SUM: 113 5054 7349 18865

But since the new implementation is more powerful than the old one - doing
things like committing/uncommitting on demand, guarding allocated blocks
etc - one could argue that the new solution does quite a lot more with
slightly less code, which I think is a good result.

---

Idle musing: it would simplify matters quite a bit if we were to unify
class space and non-class metaspace. If we keep the current narrow Klass
pointer encoding scheme, we would still need to keep the space they are
stored in contiguous. But we could use the class space for everything, in
effect dropping the "class" moniker from the name. It would have to be
larger than it is currently, of course.

Cons:
- we would have to abandon the notion of "limitless metaspace" - but if we
run with class space this has never been true anyway since the number of
classes is limited by the size of the compressed class space.
- virtual process size would go up since it now needs to be as large as the
total projected metaspace size.
- we may need to expand narrow Klass pointer encoding to include shifting,
if 4G are not enough to hold all metaspace data.

Pros:
- this would simplify a lot of code.
- this would save real (committed) memory, since we save quite a bit of
overhead.
- we could narrow-pointer-encode other metadata too, not only Klass
pointers, should that ever be interesting

... but that is not part of this JEP.

---

With this patch, we have a much stronger separation of concerns, and it
should be easy to reuse the metaspace allocator in other hotspot areas as
well. For instance, ResourceAreas and friends could be replaced by
MetaspaceArenas. They do almost the same thing. And as we have seen in the
past at SAP, the C-heap backed hotspot Arenas can be a pain since spikes in
Arena usage lingers forever as process footprint (we even once rewrote
parts of the arena code to use mmap, which is just on a more primitive
level what Metaspace does).

---

I know this is short notice, but there will be a call for interested people
tomorrow at 11AM ET. In case any potential Reviewers are interested just
drop me a short note.

---

Thank you, Thomas


[1]
http://cr.openjdk.java.net/~stuefe/jep387/review/2020-08-11/guide/reduction-small-loaders.pdf
[2]
http://cr.openjdk.java.net/~stuefe/jep387/review/2020-08-11/guide/review-guide.pdf
[3]
http://cr.openjdk.java.net/~stuefe/jep387/review/2020-08-11/guide/review-guide.html
[4]
http://cr.openjdk.java.net/~stuefe/jep387/review/2020-08-11/webrev-all/webrev/
[5]
http://cr.openjdk.java.net/~stuefe/jep387/review/2020-08-11/webrev-core/webrev/
[6]
http://cr.openjdk.java.net/~stuefe/jep387/review/2020-08-11/webrev-test/webrev/
[7]
http://cr.openjdk.java.net/~stuefe/jep387/review/2020-08-11/webrev-misc/webrev/
[8] https://bugs.openjdk.java.net/browse/JDK-8251342
[9] https://bugs.openjdk.java.net/browse/JDK-8251392


More information about the hotspot-runtime-dev mailing list