RFR: 8311648: Refactor the Arena/Chunk/ChunkPool interface [v7]

[Thomas Stuefe]

On Mon, 7 Aug 2023 19:27:49 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> Hi,
>> 
>> This PR refactors the Arena code without introducing any changes in behavior. I believe that this leads to a much clearer API and that this will in turn simplify the maintenance of this code. `Chunk` is now much more "dumb", mainly acting as a way of interrogating the chunk of data that it is holding. The `ChunkPool` gains a more prominent role, being entirely responsible for the memory allocation strategy used. `Arena` is basically unchanged, as it orchestrates the usage of these two APIs. The `chop` and `next_chop` methods are re-defined to be static inside of `Chunk`, having an object `delete` itself  is very surprising, so I'm happy to get rid of that.
>> 
>> I hope that you agree with me on these changes!
>> 
>> Cheers,
>> Johan
>
> Johan Sjölen has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 21 commits:
> 
>  - Merge remote-tracking branch 'origin/master' into rework-arena
>  - Merge remote-tracking branch 'origin/master' into rework-arena
>  - Found the bug!
>  - Is it just a missing include?
>  - Separate assignment
>  - Accidentally deleted a constructor
>  - Make Chunk NONCOPYABLE
>  - Fix style issues
>  - Dleete chunks
>  - Merge remote-tracking branch 'origin/master' into rework-arena
>  - ... and 11 more: https://git.openjdk.org/jdk/compare/c1f4595e...c688bb55

Small nits remain. Any chance you can come up with some gtests for the pool? Up to you; it would mean restructuring the code and moving the pool to a header, probably.

src/hotspot/share/memory/arena.cpp line 51:

> 49: class ChunkPool {
> 50:   // Our four static pools
> 51:   static const int _num_pools = 4;

constexpr, while you are here?

src/hotspot/share/memory/arena.cpp line 73:

> 71:   }
> 72: 
> 73:   void prune() {

Admittedly the old comment was useless, but could you add a comment explaining what happens?

Alternatively, rename the function. E.g. free_all_chunks or similar. It is not really pruning anything since it completely empties out the list (pruning suggests there is something pruned left afterwards).

src/hotspot/share/memory/arena.cpp line 277:

> 275: 
> 276:   Chunk* k = _chunk;            // Get filled-up chunk address
> 277:   _chunk = ChunkPool::allocate_chunk(len, alloc_failmode);

Odd that we would fatal out if init chunk cannot be allocated, and here we leave the OOM behavior to the caller. Is this ever called with anything else than exit_oom?

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

PR Review: https://git.openjdk.org/jdk/pull/14803#pullrequestreview-1568851267
PR Review Comment: https://git.openjdk.org/jdk/pull/14803#discussion_r1288019865
PR Review Comment: https://git.openjdk.org/jdk/pull/14803#discussion_r1288040040
PR Review Comment: https://git.openjdk.org/jdk/pull/14803#discussion_r1288053332