RFR: Implementation of JEP 387: Elastic Metaspace (round two)

[Leo Korinth]

Hi Thomas,

I have a few more comments, feel free to ignore them or incorporate them in the future as I understand you want to be able to push sometime and not do changes forever.


VirtualSpaceList.hpp

   67   // Whether this list can expand by allocating new nodes.
   68   const bool _can_expand;
   69
   70   // Whether this list can be purged.
   71   const bool _can_purge;


_can_expand and _can_purge is the same variable, it is const and
always initialized to the same value in the constructor. I think it
would be better to have just one variable, maybe one of type:

enum class VirtualSpaceType {CompressedMetaSpace, LinkedMetaSpace}

(if you like the idea, please feel free to improve on my name suggestion.)
  
  98   virtual ~VirtualSpaceList();

Why is the destructor virtual, I could find no derived classes.

VirtualSpaceList.cpp

inconsistant indenting of initializer list at line 45 and 58.

   68   // Create the first node spanning the existing ReservedSpace. This will be the only node created
   69   // for this list since we cannot expand.
   70   VirtualSpaceNode* vsn = VirtualSpaceNode::create_node(rs, _commit_limiter,
   71                                                         &_reserved_words_counter, &_committed_words_counter);
   72   assert(vsn != NULL, "node creation failed");

Maybe instead assert(vsn != NULL, "node creation can not fail and
reach this point") to ensure the invariant.  "node creation failed"
kind of implies that create_node can fail (and return) and then one
can wonder why we assert on it not happening.

  112   if (_first_node == NULL ||
  113       _first_node->free_words() == 0) {
  114
  115     // Since all allocations from a VirtualSpaceNode happen in
  116     // root-chunk-size units, and the node size must be root-chunk-size aligned,
  117     // we should never have left-over space.
  118     assert(_first_node == NULL ||
  119            _first_node->free_words() == 0, "Sanity");

This above assert just look weird as it perfectly mirrors the above if-statement.

  123     if (_can_expand) {
  124       create_new_node();
  125       UL2(debug, "added new node (now: %d).", num_nodes());
  126     } else {
  127       UL(debug, "list cannot expand.");
  128       return NULL; // We cannot expand this list.
  129     }
  130   }
  131
  132   Metachunk* c = _first_node->allocate_root_chunk();
  133
  134   assert(c != NULL, "This should have worked");
  135
  136   return c;

And I think the test for _first_node->free_words() == 0 is quite
confusing. I understand that whenever free_words() != 0 that also
free_words() >= chunklevel::MAX_CHUNK_WORD_SIZE, but why not test for
the latter? Especially as that is what is tested for in
VirtualSpaceNode::allocate_root_chunk().

  153   int num = 0, num_purged = 0;
  154   while (vsn != NULL) {
  155     VirtualSpaceNode* next_vsn = vsn->next();
  156     bool purged = vsn->attempt_purge(freelists);
  157     if (purged) {
  158       // Note: from now on do not dereference vsn!
  159       UL2(debug, "purged node @" PTR_FORMAT ".", p2i(vsn));
  160       if (_first_node == vsn) {
  161         _first_node = next_vsn;
  162       }
  163       DEBUG_ONLY(vsn = (VirtualSpaceNode*)((uintptr_t)(0xdeadbeef));)
  164       if (prev_vsn != NULL) {
  165         prev_vsn->set_next(next_vsn);
  166       }
  167       num_purged++;
  168       _nodes_counter.decrement();
  169     } else {
  170       prev_vsn = vsn;
  171     }
  172     vsn = next_vsn;
  173     num++;
  174   }

"int num" is unused except for being incremented, remove the variable.


Finally, why was it chosen that each node could carry precisely two
chunk-roots? It seems somewhat easier to have a one-to-one relation
between chunk-root and VirtualSpaceNode (when we are already so close
to one).

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()?

Thanks,
Leo