[foreign-memaccess+abi] RFR: 8263018: Improve API for lifecycle of native resources
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Fri Mar 5 11:04:51 UTC 2021
Hi Ty, thanks for giving this patch a try
On 05/03/2021 03:42, Ty Young wrote:
> 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().
It seems a case where the same memory region (which is probably created
when you instantiate the NativeInteger) is _aliased_ multiple times (via
list::get) - which seems wrong. If you create a new resource scope on
each NativeList::get, then, yes, closing something you obtain from a
list is, fundamentally, a no-op.
I believe you went down this path because you wanted to store the list
of native entities in a flattened off-heap array - so, when you do that
you have to throw all lifecyle info out of the window, and reconstruct
it from scratch on each get(). In other words, some of the information
about what's stored in the list is lost. The API cannot save you there.
I guess you would argue that ResourceScope should "just get it" that all
these aliases refer to the same thing, but ResourceScope knows nothing
about the underlying memory - it's "just" a neutral abstraction with a
bunch of cleanup action which get called on close().
Seems to me that your NativeList is akin to reading integers from a real
int* off-heap - so you can't expect much from the lifecycle machinery.
When reading from off-heap, everything gets, by default, the globalScope
assigned, which is non-closeable - maybe you should go down this path
for NativeList::get? E.g. instead of creating a resource scope out of
the blue, use the global scope? That way, a call to close() on the
retrieved element will fail.
Maybe you can make it so that a NativeList has its own ResourceScope,
and when it closes, it calls close() on all the elements that have been
added to it (but that would be expensive).
There's no silver bullet here - I don't think _any_ API design can help
much.
>
>
> 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.
Perhaps the names can be changed - but I wouldn't pull this string too
far (which you do just below).
>
>
> 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?
This is the Panama is just for interacting with C siren song; the memory
access API, while useful for interacting with off-heap memory (like C
pointers do) is not _all_ about C interop. There are users of this API
(like there are for ByteBuffer) who would just look for a way to "create
an off heap memory segment" - and will complain if they will have to
jump through 5 hops to get there (since they know can do that with
ByteBuffer.allocateDirect).
In other words, I think there's false dichotomy at play here: either we
have MemorySegment::allocateNative OR we have cstdlib bindings. It seems
to me that one can have both - and maybe ignore the tools one doesn't
care about?
If users really want to remain off heap and do operation on that off
heap string using functions in strings.h, they have an option too, which
is to extract strings.h and use the functions in there.
At which point the objection will be one between (1) I don't want to use
jextract or (2) why does every user need to re-extract the same binding
over and over? Well, maybe they won't have to - maybe some well-done,
linker-based cstdlib binding will appear in Maven Central one day, and
people can just depend on that.
But that seems to me a question for another day - what we're doing here
is providing people primitives to interact with off heap memory and
native functions to do various tasks (_one_ of them being writing a
cstdlib Java port).
>
>
> * 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?
This seems a bug. They should be completely symmetric.
>
>
> * 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?
No. Malloc returns a segment with a fresh, separate resource scope, so
that you can free things individually (that's why sharing the same scope
won't work here). The variant you are looking for is there - it's called
SegmentAllocator::ofScope - which creates an allocator out of a scope
and keeps calling MemorySegment::allocateNative(long, long, scope).
Maurizio
>
>
> 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
More information about the panama-dev
mailing list