RFR: 8251158: Implementation of JEP 387: Elastic Metaspace

Albert Yang albert.m.yang at oracle.com
Sat Aug 22 08:47:36 UTC 2020


Hi Thomas,

Thank you for preparing the review guide, which has been quite helpful 
to see the big picture, and in providing a starting point to embrace 
this large patch/code base.

## Metachunk exposing committed memory status

One thing I find odd is that chunk doesn't encapsulate committed memory 
status. In other words, a component interacting with a chuck needs to be 
aware of how much memory is committed. I will provide two examples in 
code, one is the "consumer" of `Metachunk`, the other the "producer" of 
`Metachunk`.

For the "consumer" case, `MetaspaceArena::allocate`, allocation is 
broken into two steps, 1. for committed memory, 2. for virtual memory 
(+committed memory from step 1).

```
if (!current_chunk_too_small) {
     if (!current_chunk()->ensure_committed_additional(raw_word_size)) {
     UL2(info, "commit failure (requested size: " SIZE_FORMAT ")", 
raw_word_size);
     commit_failure = true;
     }
}

// Allocate from the current chunk. This should work now.
if (!current_chunk_too_small && !commit_failure) {
     p = current_chunk()->allocate(raw_word_size);
     assert(p != NULL, "Allocation from chunk failed.");
}
```

If the "committed memory status" is properly encapsulated in 
`Metachunk::allocate`, the above code could be like:

```
if (!current_chunk_too_small) {
   p = current_chunk()->allocate(raw_word_size);
}
```

For the "producer" case, `ChunkManager::get_chunk` mostly deals with 
identifying a chunk which satisfied some constraints, but the following 
code focuses on committed bytes, which, in my opinion, is on another 
abstraction level. I believe that moving such logic to 
`Metachunk::allocate` could make code more readable.

```
// Attempt to commit the chunk (depending on settings, we either fully 
commit it or just
//  commit enough to get the caller going). That may fail if we hit a 
commit limit. In
//  that case put the chunk back to the freelist (re-merging it with its 
neighbors if we
//  did split it) and return NULL.
const size_t to_commit = Settings::new_chunks_are_fully_committed() ? 
c->word_size() : min_committed_words;
if (c->committed_words() < to_commit) {
     if (c->ensure_committed_locked(to_commit) == false) {
     UL2(info, "failed to commit " SIZE_FORMAT " words on chunk " 
METACHUNK_FORMAT ".",
         to_commit,  METACHUNK_FORMAT_ARGS(c));
     c->set_in_use(); // gets asserted in return_chunk().
     return_chunk_locked(c);
     c = NULL;
     }
}
```

## The motivation for using a buddy-allocator

It seems that `MetaspaceArena::deallocate()` never releases the memory 
to OS (memory is still committed); instead, it maintains those memory in 
a free list for future allocation. In other words, memory is never 
uncommitted until `~MetaspaceArena()`. Then, it's not obvious to me why 
a buddy-allocator is used. I was think a lazily committed would be 
enough, something like the following. Maybe I overlooked some 
constraints in the original problem spec. Any clarification/elaboration 
is appreciated, and they should probably be documented in the 
corresponding file(s).

```
struct root_chuck {
   word* _end;
   word* _committed_end;
   word* _top;

   word* alloc(size_t s_in_words) {
     if (_top + s_in_words > _end) {
       // no space for this chuck
       return nullptr;
     }
     if (_top + s_in_words > _committed_end) {
       if (!commit(_committed_end, round_up(s_in_words, 64K))) {
         // can't commit
         return nullptr;
       }
       // update commit end
       _committed_end += ...
     }
     // success path
     auto ret = _top;
     _top += s_in_words;
     return ret;
   }
};
```


## minor comments

`MetaspaceArena::retire_current_chunk()` is referenced in the some 
places, but there's no such method. Maybe stale comments.


Better to use more informative names than `_chunks`, e.g. 
`_allocated_chunks` vs `_free_chunks`, since they are quite different.

In `MetaspaceArena`,

```
// List of chunks. Head of the list is the current chunk.
MetachunkList _chunks;
```
and `ChunkManager`
```
// Freelists
FreeChunkListVector _chunks;
```
-- 
/Albert


More information about the hotspot-gc-dev mailing list