RFR: JDK-8272112: Arena code simplifications

[Thomas Stuefe]

On Mon, 9 Aug 2021 03:39:57 GMT, Kim Barrett <kbarrett 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.
>
> src/hotspot/share/memory/arena.cpp line 62:
> 
>> 60: 
>> 61:   // Remove a chunk from the pool and return it; NULL if pool is empty.
>> 62:   Chunk* remove_chunk() {
> 
> I think I prefer the old allocate/free nomenclature.  `remove_chunk` sounds like it should be discarding.  And `return_chunk` is confusing about "return".  I think allocate/free from a pool/free-list is well-understood naming.

I renamed ChunkPool::allocate() because it does not allocate anymore, it just stores existing chunks.

Is it just the naming, or do you also prefer that the old ChunkPool::allocate() actually allocated the Chunks (it called os::malloc)? I found that unnecessary for a pure cache and it duplicated the allocation code in Chunk::operator new().

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

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