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

Hi Johan,

> Hi,
> 
> @tstuefe , @coleenp, I found the bug. The issue was that `Chunk::operator new` would implicitly call the constructor of `Chunk` and thus overwrite previous content of chunks taken from the pool. I forgot to call the constructor for chunks taken from the pool. I've rewritten `allocate_chunk` so that there are two allocation branches that both end with the constructor being called via placement new.
> 

I don't get this :-)

What new call are you talking about? And in the current version, why should a constructor call be necessary if all we do is handing an existing - already initialized - chunk from and to the pool? Yes, it still carries _next from the pool, but that is soon to be overwritten when the Arena adds that chunk to its list (although setting _next to NULL in debug deliberately can't hurt).

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

PR Comment: https://git.openjdk.org/jdk/pull/14803#issuecomment-1670779672