On Mon, 20 Jan 2025 15:00:14 GMT, Maurizio Cimadamore <mcimadam...@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
