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

[Maurizio Cimadamore]

On Wed, 10 Mar 2021 11:29:09 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/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?

not sure I get the suggestion, but when looking at this my feeling was that this method was just copying some data off to some other segment which is then allocated using the provided allocator. I believe what is missing here is a check that the guy calling read() can in fact do that (e.g. if the valist is confined, is the thread the same?) - but I'm not sure that, other than the check, we need something else?

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

because in the other cases it happens implicitly via allocation

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

probably was written before the overload was added

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

yes

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

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