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