On Mon, 20 Jan 2025 13:43:35 GMT, Jorn Vernee <[email protected]> wrote:
>> Matthias Ernst has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Implementation notes.
>
> src/java.base/share/classes/jdk/internal/foreign/abi/SharedUtils.java line
> 396:
>
>> 394: long address = fromCache != 0 ? fromCache :
>> CallBufferCache.allocate(bufferSize);
>> 395: return new
>> BoundedArena(MemorySegment.ofAddress(address).reinterpret(size));
>> 396: }
>
> Looks like we're using `reinterpret` in the fallback case as well?
>
> I suggest structuring the code like this:
>
> Suggestion:
>
> final Arena arena = Arena.ofConfined();
> MemorySegment segment = CallBufferCache.acquire(arena, size);
> if (segment == null) {
> segment = arena.allocate(size);
> }
> final SegmentAllocator slicingAllocator =
> SegmentAllocator.slicingAllocator(segment);
> return new Arena() {
> @Override
> public Scope scope() {
> return arena.scope();
> }
> @Override
> public void close() {
> arena.close();
> }
> @Override
> public MemorySegment allocate(long byteSize, long byteAlignment) {
> return slicingAllocator.allocate(byteSize, byteAlignment);
> }
> };
> }
>
>
> i.e. encapsulate all the caching logic into the `CallBufferCache` class. Note
> that the 'release' action can be attached to `arena` as a cleanup action when
> calling `reinterpret`.
Was about to suggest the same. The cache should work at a segment level - e.g.
the client should call just `acquire` and get a valid segment back (no matter
how that is obtained).
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23142#discussion_r1922532547
