RFR: 8251158: Implementation of JEP 387: Elastic Metaspace

[Albert Yang]

Hi Thomas,

 > error handling is just simpler and cleaner if ChunkManager::get_chunk() never hands out 
a fully uncommitted chunk in the first place

I see. Then, when the current chunk is salvaged, it must have a positive used words, as 
the first request will always be satisfied, right? I added `assert(false)` in 
`MetaspaceArena::salvage_chunk()`, running `make test TEST='gtest:metaspace 
test/hotspot/jtreg/gtest/MetaspaceGtests.java 
test/hotspot/jtreg/runtime/Metaspace/elastic'` doesn't trigger the assertion. Is it truly 
unreachable or am I missing some tests?

```
   if (c->used_words() == 0) {
     assert(false, ""); // newly added
     UL2(trace, "salvage: returning empty chunk " METACHUNK_FORMAT ".", 
METACHUNK_FORMAT_ARGS(c));
     _chunk_manager->return_chunk(c);
     return;
   }
```

Memory allocation in metaspace involves both virtual (potentially enlarging a chunk) and 
physical (potentially committing memory) address space, and either one could fail. The 
second step, if failed, needs to undo the first step as well. Such tight-coupling is the 
main reason (to me) to not expose this as two steps to the caller. As I read it, 
`MetaspaceArena::allocate()` actually doesn't release the virtual memory used in the first 
if the second step fails. Not sure if there are any consequences or the severity.

I wonder if the problem could be simpler if chunks smaller/larger than the commit granule 
are dealt with in two different ways. This way when we deal with chunks smaller than the 
commit granule, we know it's already committed.

 > In other words, chunks can be smaller than a commit granule and that makes sense since 
for small loaders we want to share a granule among multiple loaders.

It makes sense. Somehow, I got the impression that MetaspaceArena has 1:1 mapping to 
ChunkManager, but actually all arenas share the singleton class/non-class chunk manager. 
The diagram in 
https://cr.openjdk.java.net/~stuefe/JEP-Improve-Metaspace-Allocator/review-guide/review-guide-1.0.html#2-subsystems 
was quite clear, but I think I ignored it after failing to find "SpaceManager" in the 
code. (Maybe the diagram is out of sync with the code?) Thank you for the explanation, 
which, I believe, should be included in the comments for documentation purpose. (I didn't 
find the rational for using a buddy allocator in the code comments.)

Thank you once again for the clarification; I have got a better understanding of the patch.

-- 
/Albert