On Wed, 22 Jan 2025 11:05:14 GMT, Maurizio Cimadamore <mcimadam...@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
