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-runtime-dev
mailing list