RFR: 8343394: Make MemorySessionImpl.state a stable field [v4]

[Jorn Vernee]

On Fri, 1 Nov 2024 02:44:15 GMT, Quan Anh Mai <qamai at 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