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

[Maurizio Cimadamore]

On Fri, 5 Mar 2021 16:22:00 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/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.

I think this is a typo when I rewrote the javadoc. The SegmentAllocator parameter is inferred, and is not required in the method type. So I think it already works as you expect but the javadoc is wrong.

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

I was thinking that we could have a thread local arena based allocator - which would completely eliminate synchronization issues. I just didn't get to it. But there are options we can explore.

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

I think when I looked at it in depth, having the scope didn't seem to have any effect in there - but I'll need to double check.

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

Because I want to see what happens when you call close and there are pending threads - e.g. close vs. add races

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

whoops!

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

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