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

[Matthias Ernst]

On Wed, 22 Jan 2025 11:05:14 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> src/java.base/share/classes/jdk/internal/foreign/abi/BufferStack.java line 38:
>> 
>>> 36:         @SuppressWarnings("restricted")
>>> 37:         public MemorySegment allocate(long byteSize, long byteAlignment) {
>>> 38:             MemorySegment slice = backingSegment.asSlice(offset, byteSize, byteAlignment);
>> 
>> You have re-implemented a slicing allocator (`SegmentAllocator::slicingAllocator`). I think that's probably ok, but I'll point out that slicing allocator will try to deal with alignment, and will also throw exceptions when called with non-sensical arguments.
>> 
>> A more robust way to do this would be to:
>> 1. have `reserve` pass the reserved size to `Frame`
>> 2. `Frame` will slice the segment according to offset/size
>> 3. then create a slicing allocator based on that segment
>> 4. use the slicing allocator to implement `allocate`
>> 
>> In our tests, something like this did not add any extra overhead (the allocation of the slicing allocator is escape-analyzed away)
>
> On another note: in principle if a Frame is not the latest returned in a given thread, it is not safe to allow its allocation method (and probably close too) to succeed. Consider this case:
> 
> 
> Arena arenaOuter = bufferStack.reserve(100);
> arenaOuter.allocate(16); // now offset is 16
> ...
> Arena arenaInner = bufferStack.reserve(100);
> arenaInner.allocate(16); // now offset is 32
> arenaOuter.allocate(16); // now offset is 48 // whooops
> ...
> arenaInner.close(); // now offset is 16
> arenaOuter.allocate(16); // now offset is 32
> arenaOuter.allocate(16); // now offset is 48 (overwriting!!)
> 
> 
> The last statement in this snippet effectively overwrites the memory that has been previously allocated _by the same frame_.  This is because we allow `arenaOuter::allocate` to work even after `arenaInner` has been obtained (see statement with `whooops` comment).
> 
> Now, I believe the linker implementation is "correct" and never attempts something like this - other clients might try something similar and get surprising result. So, in the general case, I believe we should think about this (although it's fine if you do not want to tackle that in this PR)

Right. Using SlicingAllocator now.

Which brings up one question about alignment of the stack frames. When the linker asks for a buffer, it also has an alignment requirement. Do we guarantee anything about the alignment of Arena.ofConfined().allocate() ? Does the linker overallocate already?

To accomodate for this, I added a frame alignment parameter as well => the stack itself is well-served by using a SlicingAllocator. For this, I added methods to query and reset the position, as well as whether it could satisfy a certain allocation request.

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

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