On Wed, 22 Jan 2025 10:43:27 GMT, Maurizio Cimadamore <[email protected]>
wrote:
>> Matthias Ernst has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> --unnecessary annotations
>
> 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)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23142#discussion_r1925132267
