On Wed, 3 Jun 2026 11:32:07 GMT, Maurizio Cimadamore <[email protected]>
wrote:
>> src/java.base/share/classes/jdk/internal/foreign/ThreadConfinedSegmentPool.java
>> line 116:
>>
>>> 114: }
>>> 115:
>>> 116: final class CachedArena implements Arena, NoInitAllocator {
>>
>> My general feeling here is that the implementation is arranged the wrong
>> way. E.g. in my mind, we have ArenaImpl, which is the type of the builtin
>> arena we return. And, if an ArenaImpl is confined, it can allocate memory
>> more cheaply, with the help of some kind of thread-backed allocator.
>>
>> I feel the right arrangement is to have a SegmentAllocator (not an Arena)
>> that returns usable regions of memory from a given thread. Maybe the
>> allocator is very low level, it computes the next pointer, and does a
>> `MemorySegment.ofAddress(ptr)` for the region. Then the ArenaImpl::allocate
>> takes that, and does a reinterpret with the correct arena and size. When the
>> confined arena closes, the memory is returned to the underlying pool.
>>
>> Since this is the builtin confined arena we're talking about, I'm not sure
>> about CachedArena -- as that looks like any other 3rd party Arena. I think
>> we can achieve tighter integration?
>
> More specifically, I think that in both the confined and shared/auto case,
> what you want is a setup like this:
>
> 1. when an arena is created, we try to acquire some memory from some pool
> 2. if we fail, then the arena behaves like before
> 3. otherwise, the first N allocations of the arena will be served by the pool
> 4. after that, we either try to acquire another pool, or fallback to default
> impl
>
> The only difference between confined and shared is where the pool lives, and
> how it is acquired.
My feeling is the same here. Looking at the patch it feels like the control is
inverted. I'd expect either ArenaImpl to be changed directly to do what
Maurizio lists here, or to have a `ConfinedArenaImpl` sub class that does these
things (but I think from internal discussion we wanted to do something that
works for shared/auto as well)
In my mind we don't want a different arena impl, we want the existing arena
impls do to something different, if that makes sense.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/31365#discussion_r3348205042
