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

Fri Mar 5 03:42:52 UTC 2021

Rebased my project on your fork and the main things I use work.

I did create a native, off-heap based ArrayList implementation for fun 
before this:

         // new list with four NativeInteger elements
         NativeList<NativeInteger> nativeList = new NativeArrayList(4, 
NativeInteger.LAYOUT);

         NativeInteger one = new NativeInteger();

         one.setValue(1);

         NativeInteger two = new NativeInteger();

         two.setValue(2);

         NativeInteger three = new NativeInteger();

         three.setValue(3);

         NativeInteger four = new NativeInteger();

         four.setValue(4);

         nativeList.add(one);
         nativeList.add(two);
         nativeList.add(three);
         nativeList.add(four);

Which ResourceScope.addOnClose helped as it's now possible to remove 
"freed" addresses from the list, there is an ambiguity as to whether or 
not underlying memory is *actually* freed. For example, if you were to 
append the above with:

         nativeList.get(1).close();

         System.out.println("ALIVE: " + two.isAlive());

two.isAlive() returns true. Does two still point to valid allocated 
memory? Because the NativeList just reconstructs the type from a 
dereferenced address, each reconstruction has a completely different 
liveliness than two or any other object returned via list.get().

Some things:

* Having generic sounding methods with non-trivial defaults isn't a good 
idea IMO. I can imagine someone calling 
MemorySegment.allocateNative(long) simply because it's the path of least 
resistance and they didn't read the documentation then finding out it's 
not at all what they wanted. API users should be forced to think about 
the choices they make when it comes to how the memory is managed, IMO.

And, as a side note, I really wish the whole allocateNative API was 
removed entirely and instead replaced with built-in c stdlib malloc 
bindings or something. There isn't really a good visual separation 
between what's native, off-heap API and non-native, on-heap API in 
Panama. Some other people IIRC have requested something akin to c stdlib 
bindings before, and there are use cases where using c stdlib would be 
useful, particularly with working with off-heap strings. For example, 
Nvidia's X Control API returns the PowerMizer(semi related to pstates) 
table in String format:

perf=0, nvclock=139, nvclockmin=139, nvclockmax=607, nvclockeditable=1, 
memclock=405, memclockmin=405, memclockmax=405, memclockeditable=1, 
memTransferRate=810, memTransferRatemin=810, memTransferRatemax=810, 
memTransferRateeditable=1 ; perf=1, nvclock=139, nvclockmin=139, 
nvclockmax=1911, nvclockeditable=1, memclock=810, memclockmin=810, 
memclockmax=810, memclockeditable=1, memTransferRate=1620, 
memTransferRatemin=1620, memTransferRatemax=1620, 
memTransferRateeditable=1 ; perf=2, nvclock=253, nvclockmin=253, 
nvclockmax=2025, nvclockeditable=1, memclock=4513, memclockmin=4513, 
memclockmax=4513, memclockeditable=1, memTransferRate=9026, 
memTransferRatemin=9026, memTransferRatemax=9026, 
memTransferRateeditable=1 ; perf=3, nvclock=253, nvclockmin=253, 
nvclockmax=2025, nvclockeditable=1, memclock=5005, memclockmin=5005, 
memclockmax=5005, memclockeditable=1, memTransferRate=10010, 
memTransferRatemin=10010, memTransferRatemax=10010, 
memTransferRateeditable=1

If this is already in an off-heap byte array, why create a Java array 
and allocating a bunch of char arrays and Strings if you could work with 
it off-heap and only allocate things on-heap when you absolutely need to?

* Why is ofShared() documented to be managed by a cleaner but 
ofConfined() isn't? I'm kinda confused here, the only difference is 
whether or not MemorySegments created from a SegmentAllocator are tied 
to a thread or not, right?

* Is the Supplier<ResourceScope> argument really needed in 
SegmentAllocator.malloc()? Why doesn't it just accept a normal 
ResourceScope variable? Could a non Supplier variant be added?

On 3/4/21 7:30 AM, Maurizio Cimadamore 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/
>
> -------------
>
> Commit messages:
>   - Fix whitespaces
>   - Rename ResourceScope::Lock
>   - Fix javadoc
>   - Slightly clarify ProgrammableInvoker::specialize
>   - Add assertion when functions are void
>   - Remove comments from TestDowncall
>   - Add overload for restricted VaList factory which takes a scope
>   - Fix TestResourceScope
>   - Fix issues with GC cleaned upcall
>   - Fix Windows VaList to add eager scope check on creation/copy
>   - ... and 58 more: https://git.openjdk.java.net/panama-foreign/compare/96c29d52...bf0ee807
>
> Changes: https://git.openjdk.java.net/panama-foreign/pull/466/files
>   Webrev: https://webrevs.openjdk.java.net/?repo=panama-foreign&pr=466&range=00
>    Issue: https://bugs.openjdk.java.net/browse/JDK-8263018
>    Stats: 5703 lines in 84 files changed: 2396 ins; 2191 del; 1116 mod
>    Patch: https://git.openjdk.java.net/panama-foreign/pull/466.diff
>    Fetch: git fetch https://git.openjdk.java.net/panama-foreign pull/466/head:pull/466
>
> PR: https://git.openjdk.java.net/panama-foreign/pull/466