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

[Maurizio Cimadamore]

On Tue, 9 Mar 2021 13:53:16 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix issue in ResourceScope::ofShared() javadoc
>
> 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?

Probably - but, to be clear - at this point (and I need to add doc on this), we are only interested about add vs. close races - not close vs. close. So, the only "bad" thing that could happen is that some other thread adds to this list while we're closing it. So I guess in this case I omitted the read acquire because the only case where you could read a DUMMY_CLEANUP is if somebody has already stored that in there (otherwise you'll see something else, and move on). But no harm in making it more robust.

Another way we could go would be to manage races in the MemoryScope impl. That is, in a shared memory scope we could have some intermediate state (e.g. ADDING) which we go through and which is mutually exclusive w.r.t. CLOSE. I didn't want to go down that path since then in ResourceList I'd have to deal with add vs. add races anyway.

> 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?

yep - we only need to check the thread. If the handle is not released then by definition the scope is alive.

> 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.

It's possible that there could be some simplification here - the code went back and forth a bit.

> 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.

ok

> 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.

I can take a look

> 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);

true!

> 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?)

This method started off very complex and now ended up much simpler - we can probably revert the various call sites and just add a shared functionality to inject the allocator.

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

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