RFR: 8287788: reuse intermediate segments allocated during FFM stub invocations [v2]

[Matthias Ernst]

On Mon, 20 Jan 2025 15:00:14 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> 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).

Moved logic to the cache to exchange MemorySegments.
Attaching cleanup breaks scalar replacement though and we see ResourceList per call. I left a note in the code, it would be trivial to attach.

>        if (segment == null) {
>           segment = arena.allocate(size);
>        }

No this wouldn't work, such a segment will be forcibly deallocated after the call, but we may want to put it into a cache slot afterwards.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23142#discussion_r1922633799