RFR: JDK-8272112: Arena code simplifications

[Thomas Stuefe]

On Sat, 7 Aug 2021 05:30:54 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

> May I please have reviews for this small change. This is pure code cleanup, issues that were identified by code review when working on JDK-8270308. 
> 
> The patch results in less code (excluding the new gtest), bit easier to maintain. I have some smaller functional improvements in the pipeline and would like to get this code cleanup out of the way first.
> 
> Changes:
> 
> - Made `ChunkPool` an automatic object. We can remove `chunkpool_init()` and instead rely on automatic initialization.
> 
> - Instead of having four named pools that need to be handled individually and selected via awkward switch constructs (see the old `Chunk::operator new()` and `::operator delete()`), we replace it with an array and a static selector (`ChunkPool* get_pool_for_size(size_t size)`).
> 
> - Removed `ChunkPool::_num_used`. It was nowhere queried, and its usefulness was questionable since these numbers don't include chunks with "special" sizes. The easiest way to obtain the footprint of arena chunks is via NMT.
> 
> - Before, we had two places where the backing memory of Chunks was allocated: in `ChunkPool::allocate()` and in `Chunk::operator new()`. The former was unnecessary. I streamlined that: removed the ability to allocate from `ChunkPool::allocate()` and renamed it to `ChunkPool::remove_chunk()`. Now Chunks are only allocated in `Chunk::operator new() `and ChunkPool just serves as Chunk cache.
> 
> - Renamed `ChunkPool::free_all_but()` to `ChunkPool::prune()` and removed the argument, made it a method-local constant (it had been a constant before in the caller of that function).
> 
> - I added a new gtest to test Chunk allocation and pooling a little.
> 
> 
> Functionally, nothing should have changed with this patch, this is pure code grooming.
> 
> ----
> 
> Tests: 
> - GHA
> - I manually tested on 64-bit and 32-bit, with and without UseMallocOnly. I also manually checked that pool usage is as expected.

Hi Kim, I addressed your requests with the last commit.

Small remarks:

- `ChunkPool::prune()` will not work with `blocksToKeep=0` (never did). I guess that is okay.

- the pattern "delete chunk and follow-ups" repeats in `Chunk::chop()`. I thought about reusing it in `ChunkPool::prune()` but `Chunk::chop()` sits above the chunk pool and we would have circularities. I then realized that semantically they are different: `Chunk::chop()` prunes an Arena, while `ChunkPool::prune()` prunes the chunk pool below. So I guess it makes sense to have them both.

- I thought about factoring out "free chunk and followers" as a new `ChunkPool::free_list()` from `ChunkPool::prune()` because strictly speaking we should do this also at destruction. But the only instances of `ChunkPool` are globals, and there is no need to spend time on cleanup when the VM exits. Is there a pattern somehow to mark a class as "its okay to leak resources since we only use this class as static globals"?

- The actual allocation of backing memory for chunks is spread over various places (`Chunk::operator new()`, `Chunk::operator delete()`, `ChunkPool::prune()`). I wondered if for clarity we should factor them out into clearly marked functions. I did not do that to keep the patch small.

-------------

PR: https://git.openjdk.java.net/jdk/pull/5040