RFR: 8251158: Implementation of JEP 387: Elastic Metaspace
Thomas Stüfe
thomas.stuefe at gmail.com
Thu Sep 3 09:19:46 UTC 2020
Hi Richard,
Thanks for your review!
Please note that code has changed since the first webrev. While that is not
a problem - most of your findings are still valid - a large number or
renamings took place (see Leo's and Coleen's mails).
I will use the new naming throughout my answers (e.g. maximal_word_size ->
MaxWordSize). Hope that is not too confusing; if in doubt pls look at the
HEAD of the sandbox repo, or just wait until the new webrev is done.
All remarks inline:
<snip>
======
>
> 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?
>
metaspace.hpp is needed for either one of MetaspaceType or MetadataType
enum. jvmciCompilerToVM.hpp needs a definition or a forward declaration for
CollectedHeap.
> ====== 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.
>
>
Good catch.
> 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?
>
>
A bit inconsistent, yes. I will remove all these asserts. They are not
really useful (if any of those were NULL we would crash in a very obvious
manner).
> 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?
>
Well, adjusted_ccs_size could have been 0 to begin with. In the greater
context I think it cannot be 0 since CompressedClassSpaceSize cannot be set
to zero. But that is non-obvious, so I prefer to leave this code in.
>
> @@ -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.
>
>
Ok
> ====== 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.
>
>
Leo and Coleen wanted this too. The new naming will be consistent and
follow hotspot naming rules: (Min|Max)WordSize.
<snip>
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.
>
It does, it uses FreeBlocks::MinWordSize. FreeBlocks consists of a tree and
a bin list. The tree is only responsible for larger blocks (larger than
what would fit into the bin list). Therefore the lower limit is only
determined by the bin list minimum word size.
Since this may be not obvious, I'll beef up the comment somewhat.
> 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.
>
Good catch.
> I'd prefer a more generic implementation that allows head->next to be
> NULL. Maybe even head == NULL.
>
>
I don't think that would be much clearer though. We probably could move
remove_from_list() up into remove_block(), but for symmetry reasons this
would have to be done for add_to_list() too, and I rather like it this way.
<snip>
> 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).
>
That is true for many asserts I add. I use asserts liberally as guard
against bit rot and as documentation. I guess this here could be considered
superfluous since the setup code is right above. I will remove that assert.
>
> 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.
>
>
I'll rewrite BlockTree to not use recursion.
> 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.
>
>
Good catch.
> 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 {
>
>
Yes, but I'd rather leave the code as it is; I think it's easier to read
that way.
> 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)
>
Ok.
<snip>
>
> 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.
>
I'll rewrite this to be non-recursive.
====== src/hotspot/share/memory/metaspace/chunkManager.cpp
>
> The slow parameter in ChunkManager::verify*() is not used.
>
>
I'll remove all "slow" params from all verifications and recode this to use
-XX:VerifyMetaspaceInterval. I think that is easier to use.
> ====== 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(), ...
>
>
Thought so too but Atomic::add seems to return the new value, not the old,
see e.g. atomic_linux_x86.hpp:
struct Atomic::PlatformAdd {
....
D add_and_fetch(D volatile* dest, I add_value, atomic_memory_order order)
const {
return fetch_and_add(dest, add_value, order) + add_value;
}
};
I also checked the callers, those few places I found which do anything
meaningful with the result seem to expect the new value. See
e.g. G1BuildCandidateArray::claim_chunk().
It is annoying that these APIs are not documented. I thought this is
because these APIs are obvious to everyone but me but looks like they are
not.
> ====== 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.
>
>
Oops. Sure, I'll remove that.
<snip>
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.
>
>
See above. In addition to what I wrote, BlockTree is an implementation
detail of FreeBlocks, so it should not matter here.
Thanks Richard.
I will work in your feedback and publish a new webrev shortly.
Cheers, Thomas
More information about the hotspot-runtime-dev
mailing list