[foreign-memaccess+abi] RFR: 8263018: Improve API for lifecycle of native resources [v2]

[Jorn Vernee]

On Fri, 5 Mar 2021 11:20:17 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This patch brings the API improvements described in [1]. Note that this is a significant reshuffle of the API, and might have non-trivial source compatibility impact; the main moves are listed below:
>> 
>> * MemorySegment is no longer AutoCloseable (ResourceScope is, and a MemorySegment *has a* ResourceScope)
>> 
>> * Resources created without an explicit scope (e.g. `MemorySegment::allocateNative(long)`) get a *default* scope, which is non-closeable and GC managed. In other words, users who do not bother with resource scopes, will get same functionalities (lifecycle-wise) that they get with the ByteBuffer API
>> 
>> * Support for custom allocators is added via the `SegmentAllocator` interface; now all API points which need to perform some allocation will accept an explicit `SegmentAllocator` parameter. Where SegmentAllocator-less overloads are provided, the assumed semantics is that of `MemorySegment::allocateNative(long, long)` which means the returned segments will be associated with the *default scope* and will **not** be closeable.
>> 
>> * The method handles generated by the linker will accept an additional leading parameter of type `SegmentAllocator` whenever the native function returns a struct by value. There is an overload which allows to statically bind an allocator at MH creation time.
>> 
>> * The NativeScope abstraction has been removed. This follows the observation that with `SegmentAllocator::arenaBounded/Unbounded` clients can already get pretty close to the functionalities provided by a NativeScope (that said, at least initially, it is likely that jextract will emit a NativeScope abstraction in the generated code, to minimize compatibility impact).
>> 
>> * As discussed in [1], the new API has a way to prevent a resource scope from being closed when operating on a resource associated with that scope; this is `ResourceScope::acquire` which returns a so called *resource scope handle* (an AutoCloseable).
>> 
>> * Access modes are gone. We only kept read-only views (as they are needed to support mapped memory). Non-closeable segments can be obtained by using the acquire API.
>> 
>> * Several methods in MemorySegment are also gone; e.g. all methods related to ownership changes (`withOwnerThread`, `share`) as well as some of the predicate methdos (e.g. `ownerThread`, which is now available through the segment's scope). The overall philosophy is that a scope is assigned to a segment on creation; the scope contains details about how the segment is to be accessed, and these details cannot be altered after the fact.
>> 
>> * `MemoryAddress::asSegmentRestricted` now takes an optional ResourceScope parameter - the scope to be associated with the returned (unsafe) segment. There is also an overload that doesn't take a ResourceScope, in which case the *global scope* (singleton, non-closeable scope) will be used. A similar argument applies to `VaList.ofAddressRestricted`.
>> 
>> * The linker will now accept a scope for the returned upcall stub segment - if no scope is provided, a default one (GC-managed, non-closeable) will be created instead. Same for `VaList::make`.
>> 
>> Overall, I think this patch makes the API cleaner by clarifying the role between lifcycles (e.g. resource scopes) and resources (segments, va lists, etc.) , w/o making clients much more verbose. We also did some internal validation (thanks Chris) to make sure that async byte buffer operation could be adjusted using the *acquire* mechanism. There are some minor outstanding issues which will need to be resolved (as part of this PR, or as a followup) - listed below:
>> 
>> * should the default scope accept custom cleanup actions? Since this scope is created internally using our cleaner factory, I think there's a possibility that user-defined cleanup action might stall the cleaner forever.
>> 
>> * should ResourceScope have a predicate to see if it's a default scope? How should it be called? In an earlier iteration I had `isCloseable` which I don't think does a good job (as a scope can also not be closeable for different reasons).
>> 
>> * Are we ok with the ResourceScope::acquire/ResourceScope.Handle names? Also note that the latter is a simple AutoCloseable, but we need a subclass because the main AutoCloseable::close throws Throwable. Which is unfortunate.
>> 
>> * What about NativeScope? Are we ok with *not* having it? Note that, without NativeScope, code like:
>> 
>> try (NativeScope scope = NativeScope.ofUnbounded()) {
>>    ...
>> }
>> 
>> becomes:
>> 
>> try (ResourceScope scope = ResourceScope.ofConfined()) {
>>    SegmentAllocator allocator = SegmentAllocator.arenaUnbounded(scope);
>>    ...
>> }
>> 
>> E.g. one extra line in the user code. I believe this is not a huge deal, in the sense that in all our example (most of which are jextract based), the body of the try with resources if pretty biggie in comparison. That said, I'm open to reintroduce NativeScope - although probably in a different form - e.g. by having a sub-interface of SegmentAllocator called BoundedAllocator, which is essentially an allocator that *has a* scope. But we can also add this later depending on our experience with using the API.
>> 
>> That's all for now. Feedback welcome!
>> 
>> [1] - https://inside.java/2021/01/25/memory-access-pulling-all-the-threads/
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix issue in ResourceScope::ofShared() javadoc

Amazing work! (it was pleasure reading this) I think this is a great step forward. MemorySegment is now a much more minimal API, and lifetime concerns are nicely contained in ResourceScope.

I tried to be very thorough with the review, since I was not that involved with the design this time around (working on upcall specialization at the same time). Also just asking some questions here and there to make sure my understanding is correct. But, a lot of the comments are just small nits.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 201:

> 199: 
> 200:     /**
> 201:      * Allocates a native segment whose base address (see {@link MemorySegment#address}) and scope which can be

Not sure this should be here?

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 145:

> 143:      * @return the downcall method handle.
> 144:      * @throws IllegalArgumentException in the case of a method type and function descriptor mismatch,
> 145:      * or if {@code type} has a prefix carrier of type {@link SegmentAllocator} but the return descriptor

So, it seems that the SegmentAllocator carrier type is expected to be present in the given MethodType? Can we just infer that a SegmentAllocator parameter should be added based on the MemorySegment return type?

I think I would prefer it that way since it keeps the mapping between the MethodType and the FunctionDescriptor more one-to-one. Also, we don't require the Addressable to be part of the MethodType for virtual functions either, so it seems a little inconsistent.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 133:

> 131:     /**
> 132:      * Obtain a foreign method handle, with the given type and featuring the given function descriptor,
> 133:      * which can be used to call a target foreign function at the given address. If the provided method

I think the note about the MemorySegment/SegmentAllocator should be a separate paragraph in the javadoc, since it doesn't apply to all linkage requests.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 217:

> 215: 
> 216:     /**
> 217:      * Allocates a native segment whose base address (see {@link MemorySegment#address}) and scope which can be

The "and scope which" seems out of place here as well.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 602:

> 600:         /**
> 601:          * Returns the resource scope associated with this instance.
> 602:          * @return the resource scope associated with this instance.

Why was this method removed? Doesn't it make sense to specify an allocator for the copy? (if it needs to allocate)

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 676:

> 674:          * <p>
> 675:          * Any native resource required by the execution of this method will be allocated in the resource scope
> 676:          * associated with this instance (see {@link #scope()}).

This seems to be redundant with the paragraph before.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 694:

> 692:          * <p>
> 693:          * If this method needs to allocate native memory for the va list, it will use
> 694:          * the given {@code NativeAllocator} to do so.

This looks out-of-date

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 697:

> 695:          * <p>
> 696:          * This method will allocate native memory to hold the elements in the va list. This memory
> 697:          * will be managed by the given {@code NativeAllocator}, and will be released when the scope is closed.

Same here. I guess some other note is needed about which allocation strategy will be used by this method.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/LibraryLookup.java line 49:

> 47:  * In cases where a client wants to create a memory segment out of a lookup symbol, the client might want to attach the
> 48:  * lookup symbol to the newly created segment, so that the symbol will be kept reachable as long as the memory segment
> 49:  * is reachable; this can be achieved by creating the segment using the {@link MemoryAddress#asSegmentRestricted(long, Runnable, ResourceScope)}.

The below example should be updated as well.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryAddress.java line 103:

> 101:      * This method is equivalent to the following code:
> 102:      * <pre>{@code
> 103:     asSegmentRestricted(byteSize, null, ResourceScope.ofShared(Cleaner.create()));

Is this code still correct? Should it be `asSegmentRestricted(bytesSize, null, ResourceScope.globalScope());` ?

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryAddress.java line 92:

> 90: 
> 91:     /**
> 92:      * Returns a new confined native memory segment with given size, and whose base address is this address. This method

Not confined, right?

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryAddress.java line 121:

> 119: 
> 120:     /**
> 121:      * Returns a new confined native memory segment with given size, and whose base address is this address. This method

Same here, not confined right? (it depends on the passed ResourceScope)

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 383:

> 381:      * <p>
> 382:      * If this segment is associated with a shared scope, calling certain I/O operations on the resulting buffer might result in
> 383:      * an unspecified exception being thrown. Examples of such problematic operations are {@link FileChannel#read(ByteBuffer)},

Is this still the case, or can acquire now be used to prevent this?

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 777:

> 775:      * Returns a native memory segment whose base address is {@link MemoryAddress#NULL} and whose size is {@link Long#MAX_VALUE}.
> 776:      * This method can be very useful when dereferencing memory addresses obtained when interacting with native libraries.
> 777:      * The returned is associated with the <em>global</em> resource scope (see {@link ResourceScope#globalScope()}).

Typo?
Suggestion:

     * The returned segment is associated with the <em>global</em> resource scope (see {@link ResourceScope#globalScope()}).

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java line 63:

> 61:  * Resource scopes can be associated with a {@link Cleaner} instance (see {@link #ofConfined(Cleaner)}) - we call these
> 62:  * resource scopes <em>managed</em> resource scopes. A managed resource scope is closed automatically once the scope instance
> 63:  * becomes <em>unreachable</em>.

Maybe this could link to https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/lang/ref/package-summary.html#reachability ?

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java line 77:

> 75:  * Confined resource scopes (see {@link #ofConfined()}), support strong thread-confinement guarantees. Upon creation,
> 76:  * they are assigned an <em>owner thread</em>, typically the thread which initiated the creation operation (see {@link #ownerThread()}).
> 77:  * After creating a confined resource scopeon, only the owner thread will be allowed to directly manipulate the resources

s/scopeon/scope
Suggestion:

 * After creating a confined resource scope, only the owner thread will be allowed to directly manipulate the resources

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java line 158:

> 156:      *     <li>this resource scope is confined, and this method is called from a thread other than the thread owning this resource scope</li>
> 157:      *     <li>this resource scope is shared and a resource associated with this scope is accessed while this method is called</li>
> 158:      *     <li>one or more locks (see {@link #acquire()}) associated with this resource scope have not been closed</li>

locks -> handles?
Suggestion:

     *     <li>one or more handles (see {@link #acquire()}) associated with this resource scope have not been closed</li>

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java line 219:

> 217:      * @param attachment an attachment object which is kept alive by the returned resource scope (can be {@code null}).
> 218:      * @param cleaner the cleaner to be associated with the returned scope. Can be {@code null}.
> 219:      * @return a new confined scope, managed by {@code cleaner}; the resulting scope is closeable if {@code closeable == true}.

Is the `closeable == true` part still needed? Seems to be from a parameter that was removed.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java line 228:

> 226:      * Create a new shared scope. The resulting scope is closeable, and is not managed by a {@link Cleaner}.
> 227:      * @return a new shared scope, managed by {@code cleaner}.
> 228:      */

Not managed by a Cleaner, but managed by `cleaner`?

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java line 249:

> 247:      * @param attachment an attachment object which is kept alive by the returned resource scope (can be {@code null}).
> 248:      * @param cleaner the cleaner to be associated with the returned scope. Can be {@code null}.
> 249:      * @return a new shared scope, managed by {@code cleaner}; the resulting scope is closeable if {@code closeable == true}.

Again, I think the `closeable == true` comment can be removed?

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/SegmentAllocator.java line 41:

> 39: 
> 40: /**
> 41:  *  This is interface models a memory allocator. Clients implementing this interface

s/is/
Suggestion:

 *  This interface models a memory allocator. Clients implementing this interface

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/SegmentAllocator.java line 443:

> 441:      * @return a new unbounded arena-based allocator
> 442:      */
> 443:     static SegmentAllocator arenaUnbounded(ResourceScope scope) {

I feel like this should include a little more explanation about the difference with `arenaBounded`, such as that when the current 'slab' is used up, a new slab will be allocated, and that allocations that are too large will be allocated outside the arena.

(In the future we could maybe add an additional parameter here to tweak the size of a slab as well)

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/SegmentAllocator.java line 452:

> 450:      * each new allocation request will return a new slice starting at the segment offset {@code 0} (alignment
> 451:      * constraints are ignored by this allocator). This can be useful to limit allocation requests in case a client
> 452:      * knows that he has fully processed the contents of the allocated segment before the subsequent allocation request

he has -> they have (gender neutrality)
Suggestion:

     * knows that they have fully processed the contents of the allocated segment before the subsequent allocation request

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/package-info.java line 67:

> 65:  * often crucial that the resources associated with a memory segment are released when the segment is no longer in use,
> 66:  * and in a timely fashion. For this reason, there might be cases where waiting for the garbage collector to determine that a segment
> 67:  * is <em>unreachable</em> is not optimal. Clients that operate under these assumptions might want to be able to programmatically

Same link to reachability could be used here: https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/lang/ref/package-summary.html#reachability

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/LibrariesHelper.java line 96:

> 94:             return new WeakReference<>(s);
> 95:         });
> 96:         return new LibraryLookupImpl(library, scopeRef.get());

I think there's still a narrow window here where `scopeRef` could be cleared, since the holder array is not necessarily alive until the end of the method AFAICS. I think this needs a reachability fence until after the call to get.
Suggestion:

        });
        ResourceScope scope = scopeRef.get();
        Reference.reachabilityFence(holder[0]);
        return new LibraryLookupImpl(library, scope);

(or maybe using a try-finally block)

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/NativeMemorySegmentImpl.java line 121:

> 119:         AbstractMemorySegmentImpl segment = new NativeMemorySegmentImpl(min.toRawLongValue(), bytesSize, defaultAccessModes(bytesSize), scope);
> 120:         if (cleanupAction != null) {
> 121:             scope.addOnClose(cleanupAction);

Why is this one not using `addOrCleanupIfFail` as well?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ResourceList.java line 55:

> 53:         public abstract void cleanup();
> 54: 
> 55:         final static ResourceCleanup DUMMY_CLEANUP = new ResourceCleanup() {

Had to look at this twice. For some reason I thought DUMMY_CLEANUP was the head of an empty list. Maybe this field could be renamed to `CLOSED_CLEANUP`?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ResourceList.java line 42:

> 40:     }
> 41: 
> 42:     final void cleanup(ResourceCleanup first) {

Should/could this be static?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ResourceList.java line 127:

> 125:                 while (true) {
> 126:                     prev = (ResourceCleanup) FST.getAcquire(this);
> 127:                     // no need to check for DUMMY, since only one thread can get here!

I see this comment is true since `MemoryScope::justClose` is called just before this, which for a shared scope takes care of the races. This also seems to be based on the assumption that a shared scope is always used together with a shared ResourceList and vice-versa. But, in the current design it still seems possible to mix up the two?

Would it make sense to change the grouping of classes like so:

class MemoryScope {
    ...
    static class ResourceList {...}
}

class ConfinedScope {
    ...
    private static class ConfinedResourceList {...}
}

class SharedScope {
    ...
    private static class SharedResourceList {...}
}

That way it seems more clear that ConfinedResourceList and SharedResourceList are implementation details of ConfinedScope and SharedScope respectively?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ResourceList.java line 133:

> 131:                 }
> 132:                 cleanup(prev);
> 133:             }

I think an exception should be thrown along the `else` branch. Since there seems to be no way that the list could already have been closed at this point? (Would rather make sure and throw an exception here).

Maybe just turn it into more of an assert style if?

if (fst == ResourceList.DUMMY_CLEANUP)
    throw new IllegalStateException("Already closed?");

Same for the confined one.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ResourceList.java line 122:

> 120: 
> 121:         void cleanup() {
> 122:             if (fst != ResourceCleanup.DUMMY_CLEANUP) {

Shouldn't this be a volatile/acquire read?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/MemoryScope.java line 255:

> 253:             public void close() {
> 254:                 checkValidState(); // thread check
> 255:                 if (!released) {

`released` flag should be updated as well here? (Otherwise it's possible to release the same handle twice, without it being a no-op the second time AFAICS).

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/MemoryScope.java line 360:

> 358:             @Override
> 359:             public void close() {
> 360:                 if (released.compareAndSet(false, true)) {

No need to check valid state here? (like with the confined handle). I guess the check in the confined cases is just to check the current thread?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/Binding.java line 263:

> 261:          * Create a binding context from given native scope.
> 262:          */
> 263:         public static Context ofBoundedAllocator(ResourceScope scope, long size) {

This factory always seems to be passed the result of `ResourceScope.ofConfined()` which ends up looking a little confusing in the caller, since it seems that the scope is passed to this factory but never closed. I think it would be better to create the confined scope in this factory method instead of passing it in as an argument.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/Binding.java line 284:

> 282:          * the context's allocator is accessed.
> 283:          */
> 284:         public static Context ofScope(ResourceScope scope) {

Similarly here, this will only be called with `ResourceScope.ofConfined` as argument. I think it's better to create the scope as part of this factory method.

I think in terms of specialization, this will only affect SharedUtils::wrapWithAllocator. I'll leave a comment there as well.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/SharedUtils.java line 266:

> 264:         }
> 265: 
> 266:         throw new IllegalArgumentException("Size too large: " + size);

Looks like a merge artifact (I changed this for upcalls). Please revert this line to retain the more specific exception message.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/SharedUtils.java line 397:

> 395:         specializedHandle = tryFinally(specializedHandle, closer);
> 396:         specializedHandle = collectArguments(specializedHandle, allocatorPos, MH_MAKE_SCOPE);
> 397:         return specializedHandle;

Right, so here the last line creates a ResourceScope that is than passed to both the closer which closes it, as well as the target MH, which wraps it in a Binding.Context and then uses it.

I think instead of creating the scope separately, it should be done as part of the Binding.Context factory methods. In this method the closer would then accept a Binding.Context and close that instead of a ResourceScope.

The scopeFilter should be unneeded in that case, CLOSE_SCOPE would become CLOSE_CONTEXT, and MAKE_SCOPE would be MAKE_CONTEXT, which points to one of the Binding.Context factories (though a similar if/else structure as for scopeFilter would be needed to select the appropriate Context factory).

How does that sound?

I wouldn't mind taking care of that at a later time either though.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/SharedUtils.java line 390:

> 388:         } else {
> 389:             // this path is probably never used now, since ProgrammableInvoker never calls this routine with bufferCopySize == 0
> 390:             scopeFilter = insertArguments(identity(Binding.Context.class), 0, Binding.Context.DUMMY);

This can use `constant`
Suggestion:

            scopeFilter = constant(Binding.Context.class, Binding.Context.DUMMY);

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/SharedUtils.java line 479:

> 477:         handle = SharedUtils.unboxVaLists(type, handle, MH_unboxVaList);
> 478:         return handle;
> 479:     }

The `descriptor` argument here is unused. But, looking also at how this is called, passing in a downcallFactory here seems unnecessary. It seems like the caller could create the downcall handle based on the MethodType it has, and then pass the type together with the created MethodHandle to this method for further adaptation? (This might have been a left-over from before we had virtual function support?)

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/aarch64/AArch64Linker.java line 61:

> 59:                 MethodType.methodType(MemoryAddress.class));
> 60:             MH_boxVaList = MethodHandles.insertArguments(lookup.findStatic(AArch64Linker.class, "newVaListOfAddress",
> 61:                 MethodType.methodType(VaList.class, MemoryAddress.class, ResourceScope.class)), 1, ResourceScope.globalScope());

Pre-existing; this is a good reminder that VaList instances passed to upcalls is not tied to the scope of the upcall, which it really should be.

Not sure how to fix it with the current code shape though, so I'll file a followup to take care of it later.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/aarch64/AArch64VaList.java line 312:

> 310:                     consumeGPSlots(1);
> 311: 
> 312:                     try (ResourceScope scope = ResourceScope.ofConfined()) {

This seems like it might be confined to another thread than before now (if the access is happening from a different thread than the one that created the VaList).

I think this could be solved by adding an `asConfined` overload that explicitly takes a thread to confine to, and then here do:

try (ResourceScope scope = ResourceScope.ofConfined(segment.scope().ownerThread())) {

WDYT?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/x64/sysv/CallArranger.java line 33:

> 31: import jdk.incubator.foreign.MemoryLayout;
> 32: import jdk.incubator.foreign.MemorySegment;
> 33: import jdk.incubator.foreign.SegmentAllocator;

Seems spurious?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/x64/sysv/SysVVaList.java line 233:

> 231:             return switch (typeClass.kind()) {
> 232:                 case STRUCT -> {
> 233:                     try (ResourceScope localScope = ResourceScope.ofConfined()) {

Same here (and below). It seems like this could be confined to a different thread than the one that owns the VaList now.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/x64/windows/WinVaList.java line 167:

> 165:     public VaList copy() {
> 166:         ((MemoryScope)scope).checkValidStateSlow();
> 167:         return new WinVaList(segment, scope);

Cool! This seems much cleaner than what we had before :) (where the original VaList could be cleaned up, freeing the underlying buffer, but the copy could still attempt access).

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/x64/windows/WinVaList.java line 181:

> 179: 
> 180:         public Builder(ResourceScope scope) {
> 181:             ((MemoryScope)scope).checkValidStateSlow();

How come this state check only happens in the Windows VaList Builder implementation?

test/jdk/java/foreign/TestAddressHandle.java line 66:

> 64:         VarHandle longHandle = MemoryLayouts.JAVA_LONG.varHandle(long.class);
> 65:         try (ResourceScope scope = ResourceScope.ofConfined()) {
> 66:             MemorySegment segment = MemorySegment.allocateNative(8, 8, scope);

Alignment argument is no longer needed right? (there's an overload that takes just a size and scope). Or was there a pre-existing problem with the test?

test/jdk/java/foreign/TestDowncall.java line 88:

> 86:         MethodHandle mh = abi.downcallHandle(addr, allocator, type, descriptor);
> 87:         mh = mh.asSpreader(Object[].class, args.length);
> 88:         Object res = mh.invoke(args);

Could use `invokeWithArguments` here instead of making the spreader first I think.

test/jdk/java/foreign/TestNativeScope.java line 51:

> 49: import static org.testng.Assert.*;
> 50: 
> 51: public class TestNativeScope {

Should this test be renamed? (e.g. to TestSegmentAllocator)

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ArenaAllocator.java line 34:

> 32: 
> 33:     @Override
> 34:     public synchronized MemorySegment allocate(long bytesSize, long bytesAlignment) {

The synchronization here is a bit unfortunate I guess... Especially since the scope might be confined. We use this allocator as well for by-value struct copies.

Can we maybe change this to an explicit lock instead of using `synchronized` and then only use the lock when the scope we get is not confined?

(can save that for another day as well though)

test/jdk/java/foreign/TestResourceScope.java line 114:

> 112:                     // already closed - we need to call cleanup manually
> 113:                     acc.addAndGet(delta);
> 114:                 }

Instead of the try/catch, can't you move the calls to Thread::join to before you call `scope.close()` instead?

test/jdk/java/foreign/TestResourceScope.java line 181:

> 179:             }).start();
> 180:         }
> 181: 

Same here, why not just join all the threads first before calling close? (and get rid of the try/catch and retry logic).

test/micro/org/openjdk/bench/jdk/incubator/foreign/Upcalls.java line 29:

> 27: import jdk.incubator.foreign.MemoryAddress;
> 28: import jdk.incubator.foreign.CLinker;
> 29: import jdk.incubator.foreign.ResourceScope;

Spurious import?

test/micro/org/openjdk/bench/jdk/incubator/foreign/libCallOverheadJNI.c line 61:

> 59:     void *buf = (void*)(*env)->GetDirectBufferAddress(env, pointBuf);
> 60:     free(buf);
> 61: }

These seem unused?

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

PR: https://git.openjdk.java.net/panama-foreign/pull/466