[foreign-memaccess+abi] RFR: 8296417: Make memory session a pure lifetime abstraction [v3]

[Jorn Vernee]

On Fri, 4 Nov 2022 19:02:27 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Add description
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix vector tests

src/java.base/share/classes/java/lang/foreign/Arena.java line 63:

> 61:     /**
> 62:      * Creates a native memory segment with the given size (in bytes), alignment constraint (in bytes) associated with
> 63:      * this memory session. The {@link MemorySegment#address()} of the returned memory segment is the starting address of

Suggestion:

     * the memory session of this arena. The {@link MemorySegment#address()} of the returned memory segment is the starting address of

src/java.base/share/classes/jdk/internal/foreign/MemorySessionImpl.java line 154:

> 152:     }
> 153: 
> 154:     public Thread ownerThread() {

No longer `final`, but it is not overridden AFAICS. Maybe just keep `final`?

src/java.base/share/classes/jdk/internal/foreign/NativeMemorySegmentImpl.java line 34:

> 32: import java.util.Optional;
> 33: 
> 34: import jdk.internal.foreign.MemorySessionImpl.ResourceList.ResourceCleanup;

Spurious import?
Suggestion:

test/jdk/java/foreign/SafeFunctionAccessTest.java line 62:

> 60:         MemorySegment segment;
> 61:         try (Arena arena = Arena.openConfined()) {
> 62:             segment = MemorySegment.allocateNative(POINT, arena.session());

I think this can be simplified:
Suggestion:

            segment = arena.allocate(POINT);

There are many other occurrences of this pattern in the tests as well.

test/jdk/java/foreign/TestMemorySession.java line 275:

> 273: 
> 274:     @Test
> 275:     public void testConfinedSessionWithImplicitDependency() {

Is it not possible to keep these 2 tests? Might be nice as a stress test for a naked acquire.

test/jdk/java/foreign/TestSegmentAllocators.java line 90:

> 88:                     } catch (IndexOutOfBoundsException | IllegalStateException ex) {
> 89:                         //failure is expected if bound
> 90:                         assertTrue(isBound);

Not sure I get this `catch`. AFAICS only a slicing allocator is bound, which should result in `NULL` being returned, not an exception, right?

test/jdk/java/foreign/TestSegmentAllocators.java line 501:

> 499:     static Object[][] allocators() {
> 500:         return new Object[][] {
> 501:                 { SegmentAllocator.implicitAllocator() },

I guess this case could be replaced with `SegmentAllocator.nativeAllocator(MemorySession.implicit())`

test/micro/org/openjdk/bench/java/lang/foreign/LoopOverPollutedSegments.java line 73:

> 71:         arena = Arena.openConfined();
> 72:         nativeSegment = MemorySegment.allocateNative(ALLOC_SIZE, 4, arena.session());;
> 73:         nativeSharedSegment = MemorySegment.allocateNative(ALLOC_SIZE, 4, arena.session());; // <- This segment is not shared!

Looks like this tries to test pollution with when a mix of confined and shared sessions are used. Maybe a good time to fix this to use a shared arena?

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

PR: https://git.openjdk.org/panama-foreign/pull/750