8263018: Improve API for lifecycle of native resources

[mcimadamore]

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/

---

Progress

• Change must not contain extraneous whitespace
• Change must be properly reviewed

Issue

• JDK-8263018: Improve API for lifecycle of native resources

Reviewers

• Jorn Vernee (@JornVernee - Committer)
• Paul Sandoz (@PaulSandoz - Committer) ⚠️ Review applies to d02e86f

Download

To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/panama-foreign pull/466/head:pull/466
$ git checkout pull/466

To update a local copy of the PR:
$ git checkout pull/466
$ git pull https://git.openjdk.java.net/panama-foreign pull/466/head

[openjdk]

⚠️ @mcimadamore This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

[mcimadamore]

Latest javadoc here:
http://cr.openjdk.java.net/~mcimadamore/panama/resourceScope-javadoc_v5/javadoc/jdk/incubator/foreign/package-summary.html

[mcimadamore]

Latest javadoc here:
http://cr.openjdk.java.net/~mcimadamore/panama/resourceScope-javadoc_v6/javadoc/jdk/incubator/foreign/package-summary.html

[mcimadamore]

A possible, improved naming for the two segment allocators:

SegmentAllocator.of(MemorySegment) -> SegmentAllocator.slicing(MemorySegment)
SegmentAllocator.of(ResourceScope) -> SegmentAllocator.scoped(ResourceScope)

What do you think?

[PaulSandoz]

A possible, improved naming for the two segment allocators:

SegmentAllocator.of(MemorySegment) -> SegmentAllocator.slicing(MemorySegment)
SegmentAllocator.of(ResourceScope) -> SegmentAllocator.scoped(ResourceScope)

What do you think?

Yes, that's better. I find it hard to come up with a better concise name for the former, allocation returns a sliced prefix of the given segment, thus allocations are aliased, which is fundamentally different to all the other cases.

[mcimadamore]

A possible, improved naming for the two segment allocators:
SegmentAllocator.of(MemorySegment) -> SegmentAllocator.slicing(MemorySegment)
SegmentAllocator.of(ResourceScope) -> SegmentAllocator.scoped(ResourceScope)
What do you think?

Yes, that's better. I find it hard to come up with a better concise name for the former, allocation returns a sliced prefix of the given segment, thus allocations are aliased, which is fundamentally different to all the other cases.

One option which I still find kind of appealing, is to add instance methods to connect the various APIs. E.g. instead:

SegmentAllocator.of(scope) -> scope.toAllocator();
SegmentAllocator.of(segment) -> segment.toSegment();

I think these conversions are great in certain contexts: think of a client slicing a segment and wanting to put the result of a native call in a certain place:

MemorySegment segment = ...
MemorySegment out = segment.asSlice(...);
linkerHandle.invokeExact(out.toAllocator(), ...);

Similarly, if you have an resource scope and simply want an allocator that returns segments attached to that scope:

try (ResourceScope scope = ...) {
   linkerHandle.invokeExact(scope.toAllocator(), ...);
}

I find both more concise that their static variants, but also (crucially) easier to explain. What we lose if we go down that path is "independence" between the various abstractions - in the sense that a ResourceScope is no longer Panama-neutral.

[mcimadamore]

I've been doing some validation, e.g. port jextract to the new API:

https://github.com/openjdk/panama-foreign/compare/foreign-jextract...mcimadamore:jextract+resourceScope?expand=1

I think overall the results is neat. I liked how the new API doesn't force me to have a scope for everything (there are many cases where we need to create a segment on the fly, as a step to be able to call a function once) - simple cases can just rely on cleaner and the GC, thus simplifying the code. At the same time, complex cases where lifecycle of multiple resources is linked, can be expressed way more clearly, since the user is now in control of the previously hidden "scope" component.

This patch adds overloads to native function wrappers returning structs, so that users can pass a NativeScope if they wish allocation to occur in that scope.

To minimize disruption, jextract is generating NativeScope in the target package; I think some abstraction linking together ResourceScope and SegmentAllocator would be nice to have for the native use case, but we're unsure as to what the best way to expose this is.

One option would be to have a simple subtype of SegmentAllocator called ScopedSegmentAllocator - which expresses the notion of "I'm allocator that has its own scope". I think this will also be useful for integration with Jim's QBA allocator.

Another option would be to associate an optional scope to every SegmentAllocator. That is, add a default method to SegmentAllocator:

default Optional<ResourceScope> allocatorScope() { returns Optional.empty(); }

Honestly, now that I look back at this, it doesn't seem as bad as I originally thought - most of the current allocators actually already have a natural scope - the only exception being malloc() - which doesn't have a scope, since all the returned segments are independent. Well, almost - you would at least still need to have SegmentAllocator <: AutoCloseable:

try (var allocator = SegmentAllocator.arenaUnbounded(ResourceScope.ofConfined)) {
    
)

Now, while SegmentAllocator::close can be specified to just do allocatorScope().close() and nothing more, there is a question as to whether an API that has sometimes closeable allocators is a good thing. It seems like, ultimately, having a sub-interface of SegmentAllocator would be cleaner - although I suspect the extra abstraction might look a tad dubious.

[PaulSandoz]

One option which I still find kind of appealing, is to add instance methods to connect the various APIs. E.g. instead:

SegmentAllocator.of(scope) -> scope.toAllocator();
SegmentAllocator.of(segment) -> segment.toSegment();

I think that works very well for SegmentAllocator.of(segment), and implementation it could be quite efficient (same concrete class if need be).

Less sure about SegmentAllocator.of(scope) -> ResourceScope.toAllocator(), FWIW I noticed this is a subset of malloc:

ResourceScope scope = ...
SegmentAllocator fixedScopeAllocator = SegmentAllocator.malloc(() -> scope);

There might also be some interconnections with ScopedSegmentAllocator? Seems like we either want to compose through inheritance or have some bi-direction between the two abstractions.

[mcimadamore]

Less sure about SegmentAllocator.of(scope) -> ResourceScope.toAllocator(), FWIW I noticed this is a subset of malloc:

ResourceScope scope = ...
SegmentAllocator fixedScopeAllocator = SegmentAllocator.malloc(() -> scope);

There might also be some interconnections with ScopedSegmentAllocator? Seems like we either want to compose through inheritance or have some bi-direction between the two abstractions.

Good point - if we had a ScopedSegmentAllocator abstraction, I believe creating an allocator from a scope would not be as important (if clients need both an allocator and a scope, well they can just allocate a ScopedSegmentAllocator).

[mcimadamore]

Less sure about SegmentAllocator.of(scope) -> ResourceScope.toAllocator(), FWIW I noticed this is a subset of malloc:
ResourceScope scope = ...
SegmentAllocator fixedScopeAllocator = SegmentAllocator.malloc(() -> scope);
There might also be some interconnections with ScopedSegmentAllocator? Seems like we either want to compose through inheritance or have some bi-direction between the two abstractions.

Good point - if we had a ScopedSegmentAllocator abstraction, I believe creating an allocator from a scope would not be as important (if clients need both an allocator and a scope, well they can just allocate a ScopedSegmentAllocator).

I made an attempt at this - captured in the following javadoc:

http://cr.openjdk.java.net/~mcimadamore/panama/resourceScope-javadoc_v6_exp/

There is a new ScopedMemoryAllocator - and some of the existing factory now return that. Note that now the difference between SegmentAllocator.malloc and SegmentAllocator.scoped is cleared, as only the latter gives you a ScopedSegmentAllocator.

All the arena allocators are also ScopedSegmentAllocators.

The recycling allocator has been moved off to the side in MemorySegment::toAllocator.

Is this an improvement? While more verbose than the old native scope, I think there's a lot more composition going on here, as now users can select full spectrum of { shared, confined } x { bounded vs. unbounded allocation } x { explicit vs. implicit close } - the former NativeScope used to be a point (although common) in this space: confined x { bounded vs. unbounded allocation } x explicit close.

[mcimadamore]

Small observation: I realized that all segment allocators can be associated with a scope. For allocators in which the scope is not important, we can just return the global scope.

In other words, associating a scope to an allocator is mostly harmless - either the allocator does have a lifecycle (and that is managed by the scope), or the allocator does not have one (in which case a neutral scope like global scope can be used).

This might address the problem of having a single entity which is connected to both an allocator and a scope - but it doesn't solve the usability problem of the try-with-resources. Unless we make SegmentAlocator AutoCloseable. But, if we make SegmentAllocator auto-closeable on the basis that an allocator has-a scope - then, on that basis it seems that even MemorySegment, VaList, ... should join for the ride? This seems like a slippery slope.

[mcimadamore]

To bring this discussion closer to actual use cases - the thing that worries me is examples like these:

          try (var url = toCString(urlStr)) {
               curl_easy_setopt(curl, CURLOPT_URL(), url.address());
               int res = curl_easy_perform(curl);
               if (res != CURLE_OK()) {
                   String error = toJavaStringRestricted(curl_easy_strerror(res));
                   System.out.println("Curl error: " + error);
                   curl_easy_cleanup(curl);
               }
           }

(this can be found in the list of jextract samples, in [1]).

This code just needs a native string (there is a similar case in the clang implementation used by jextract). Now, there are many ways to rewrite that code (for simplicity I'll just focus on the first two lines):

1. remove the try with resource - and just use "default" scope (which is managed by the GC):

var url = toCString(urlStr);
curl_easy_setopt(curl, CURLOPT_URL(), url.address());

But there's a sneaky problem here: the function uses the address projection of the string segment - meaning that, at least in principle, the string segment can become unreachable (and hence freed) before the native function is executed. This seems bad - and to avoid that, explicit resource deallocation seems preferrable (unless we want to start playing with reachability fences).

2. Use a native scope - after all this is jextracted code:

try (NativeScope scope = NativeScope.unboundedScope())) {
    var url = toCString(urlStr, scope.scope());
    curl_easy_setopt(curl, CURLOPT_URL(), url.address());
    ...
}

This works as expected. There are two issues here: non-jextracted code cannot do this (as there's no NativeScope in the API); and, arguably, using an unbounded native scope just to allocate a single string seems overkill. That will probably allocate more memory than required, to name one.

3. Use ResourceScope + SegmentAllocator

try (ResourceScope scope = ResourceScope.ofConfined()) {
    var url = toCString(urlStr, SegmentAllocator.scoped(scope));
    curl_easy_setopt(curl, CURLOPT_URL(), url.address());
    ...
}

This works, and has the same semantics as the code that it's replacing.

Now maybe (3) is the best we can do - but I can't help but feeling that code like this will be common (e.g. converting a string on the fly and passing it to some native library) and having to create a scope and an allocator to be able to do that (so that the right semantics occurs) seems unfortunate.

We could add an overload in toCString to also accept a ResourceScope (in which case the scoped allocator will be used) - this will simplify the code a bit:

try (ResourceScope scope = ResourceScope.ofConfined()) {
    var url = toCString(urlStr, scope);
    curl_easy_setopt(curl, CURLOPT_URL(), url.address());
    ...
}

But that also means that each API point accepting a SegmentAllocator will likely need a similar overload.

[1] - https://github.com/openjdk/panama-foreign/blob/foreign-jextract/doc/panama_jextract.md

[mcimadamore]

I've updated the branch. The main change is that now methods in CLinker have an overload accepting a ResourceScope - e.g. whenever we took an allocator, there is also a corresponding overload that takes only a scope. This way the user has a nice and gentle progression through the API complexity:

1. no scopes - everything is GC-managed (a la byte buffer)
2. deterministic closing with scopes - user creates a bunch of ResourceScope and all memory allocation is associated with such scopes
3. custom allocators - the user is not happy with default allocation mechanism - so, in addition to create a scope, a segment allocator will also be created.

While (1) and (2) will cover most of the "one-liner" examples, more realistic tests, such as those found in jextract will probably go for (3) [as they do today].

From here, we have a couple of API extensions that we can evaluate and add later (they can be added in a compatible fashion):

a. Make all entities that have a scope should also have a close() method- this means that, instead of scope().close(), users can say close(), and use a TWR
b. consider if the above entities should also be AutoCloseable
c. Consider whether all segment allocators should be associated with a scope. We could add a default method returning ResourceScope.globalScope so that compatibility is not affected.
d. if we do (c) maybe we could add more factories overloads e.g. an arenaBounded which takes no scope, but creates the scope in a single shot.

Note that in the latest change I've converted StdLib test not to use NativeScope. I think the result is uniform and pleasing, and I didn't find myself missing it:

https://github.com/openjdk/panama-foreign/blob/d02e86f9fad8ad4ed17d84485e9fd198aa9eea73/test/jdk/java/foreign/StdLibTest.java

I've also added some ResourceScope overloads to jextract:

https://github.com/mcimadamore/panama-foreign/tree/jextract%2BresourceScope

And tried to convert my pet OpenGL application not to use NativeScope and again, it's all pretty readable.

[PaulSandoz]

This is looking very good.

[PaulSandoz]

Potentially MemoryScope could implement SegmentAllocator then there is no need for this method?

And then on SegmentAllocator:

    static SegmentAllocator scoped(ResourceScope scope) {
        return (MemoryScope) Objects.requireNonNull(scope);
    }

[mcimadamore]

Yes - I guess I didn't want to blur the two concepts too much - if we go down that path I think users will be able to cast a ResourceScope into a SegmentAllocator?

[PaulSandoz]

They could, but they should not since it's an implementation detail. (FWIW I don't think there is any impact to the integrity of the implementation.)

[mcimadamore]

Addressed latest comments. I also added two new static methods, one to obtain a new "default scope" (that is, the implicit scope that is created when you call e.g. MemorySegment::allocateNative), and also the "default allocator" - e.g. an allocator that returns segments based on their own default scopes.

javadoc here:
http://cr.openjdk.java.net/~mcimadamore/panama/resourceScope-javadoc_v7/javadoc/jdk/incubator/foreign/package-summary.html

[JornVernee]

FWIW, all the changes still look good to me. I like the new names for the allocator factory methods and that overloads were added to CLinker that take a ResourceScope as well.

[mcimadamore]

/integrate

[openjdk]

@mcimadamore Since your change was applied there have been 304 commits pushed to the foreign-memaccess+abi branch:

• 387ae8b: Automatic merge of master into foreign-memaccess+abi
• f9d8b77: Automatic merge of jdk:master into master
• 57497ab: 8263821: Remove unused MethodTypeForm canonicalization codes
• 4d51a82: 8263818: Release JNI local references in get/set-InetXXAddress-member helper functions of net_util.c
• 701fd9d: 8262476: Add filter to speed up CompileCommand lookup
• 454af87: 8263185: Mallinfo deprecated in glibc 2.33
• d24e4cf: 8263481: Specification of JComponent::setDefaultLocale doesn't mention that passing 'null' restores VM's default locale
• 1a21f77: 8263482: Make access to the ICC color profiles data multithread-friendly
• d185655: 8263832: Shenandoah: Fixing parallel thread iteration in final mark task
• 434a399: 8260274: Cipher.init(int, key) does not use highest priority provider for random bytes
• ... and 294 more: https://git.openjdk.java.net/panama-foreign/compare/96c29d5271d655c1a967ffd67041b21da056d503...foreign-memaccess+abi

Your commit was automatically rebased without conflicts.

Pushed as commit 4192f7b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.