On Fri, 1 Nov 2024 02:44:15 GMT, Quan Anh Mai <qa...@openjdk.org> wrote:
>> Hi,
>>
>> This patch makes `MemorySessionImpl.state` a `Stable` field so that liveness
>> check of non-closeable scopes such as the global scope can be elided.
>>
>> Currently, the `state` field is overloaded with 2 responsibilities, to act
>> as a communication device between `close` and `checkValidState`, as well as
>> a communication device between `close`, `acquire`, and `release`. This patch
>> separates those concerns into `state` and `acquireCount`, allowing `state`
>> to be marked as `@Stable`.
>>
>> With the patch, in `MemorySegmentGetUnsafe`, `panama` is able to be on par
>> with `unsafe`:
>>
>> Benchmark Mode Cnt Score Error Units
>> MemorySegmentGetUnsafe.panama avgt 30 0.340 ± 0.008 ns/op
>> MemorySegmentGetUnsafe.unsafe avgt 30 0.332 ± 0.004 ns/op
>>
>> For reference this is the results without this patch:
>>
>> Benchmark Mode Cnt Score Error Units
>> MemorySegmentGetUnsafe.panama avgt 30 0.420 ± 0.019 ns/op
>> MemorySegmentGetUnsafe.unsafe avgt 30 0.329 ± 0.003 ns/op
>>
>> Please kindly review, thanks very much.
>
> Quan Anh Mai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> wait for the close operation to complete on acquire failures
src/java.base/share/classes/jdk/internal/foreign/MemorySessionImpl.java line 62:
> 60: /**
> 61: * The value of the {@code state} of a {@code MemorySessionImpl}. The
> only possible transition
> 62: * is OPEN -> CLOSED. As a result, the states CLOSED and UNCLOSEABLE
> are stable. This allows us
Suggestion:
* is OPEN -> CLOSED. As a result, the states CLOSED and NONCLOSEABLE are
stable. This allows us
src/java.base/share/classes/jdk/internal/foreign/SharedSession.java line 101:
> 99: }
> 100: return alreadyClosed();
> 101: }
It would be good to add a test that tries to trigger this use case. e.g. have 2
threads which share a shared arena. One will try to close the arena, while the
other will try to pass a segment allocated from the arena to a downcall.
test/micro/org/openjdk/bench/java/lang/foreign/LoopOverRandom.java line 65:
> 63: @Setup
> 64: public void setup() {
> 65: indices = new Random().ints(0,
> ELEM_COUNT).limit(ELEM_COUNT).toArray();
Please use a fixed seed here, to avoid instability due to data being different.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21810#discussion_r1828237651
PR Review Comment: https://git.openjdk.org/jdk/pull/21810#discussion_r1828241119
PR Review Comment: https://git.openjdk.org/jdk/pull/21810#discussion_r1828247752
