[foreign-memaccess+abi] RFR: API refresh - part two

[Maurizio Cimadamore]

On Tue, 5 Oct 2021 20:54:34 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> After playing a bit more with the new API changes, I realized there were issues with some of the solutions proposed in [1].
> I will add more details in a separate comment, to keep the header of this PR compact.
> 
> [1] - https://git.openjdk.java.net/panama-foreign/pull/576

Below is a summary of the changes; a javadoc for the proposed API is available in [3]:

* enabling cleaners by default is good on paper, but the way the factories are structured, makes it easy for users to do the wrong thing (e.g. the factory that seems like the one you want doesn't do what you expect). This led to an issue in the linker implementation [1]. This patch reverts the factories to what they were before; note we still do not have the concept of
implicit scope - but we do have a factory (named `newImplicitScope()` as in the 17 API) which returns a shared scope backed by an internal cleaner, for usability.

* We have explored several ways to make `ResourceScope` and `SegmentAllocator` more connected - but, after playing with several options, it seems like the keeping the two abstractions separate is better. We realized that, while it is tempting for a client to create an allocator, and then just passing it around, forgetting about scopes - the reality is more complex: at the end of the day, `SegmentAllocator` is a glorified `Supplier<MemorySegment>`, and it makes sense for an API to accept one only in cases where the API wants to know **where** to store some results which will be consumed elsewhere (by the caller). This is the case of downcall method handles, and `VaList::nextArg(GroupLayout, SegmentAllocator)`. But in more complex cases where a library needs to allocate memory for its own internal purposes, doing so using the client's allocator is risky: the client might be providing an allocator of the wrong kind (e.g. one that returns on-heap segments, or which alias memory). It is a
  far better option for the library to have its own allocator, which will allocate segments in the client's scope (this is possible, for instance, with the memory pool API in [2]). Once this became clearer, a lot of the pressure for trying to control both `ResourceScope` and `SegmentAllocator` with a single abstraction went away really. And, since `SegmentAllocator` is an open interface, nothing prevents clients from creating an `AutoCloseable` allocator which allocates a scope (internally). In fact, `jextract` used to do that (and called it `NativeScope`) - but we later dropped that, as using regular API methods was serviceable enough in all the samples we tried.

* The API generally takes the shared/confined state of scopes too seriously; for instance, we try to make sure that an arena allocator created with a shared scope should be thread-safe. This is generally a siren song - creating an arena allocator from a shared scope should simply mean that the allocator will return segments from that (shared) scope. This patch rectifies
 that, and also comes up with a more uniform naming scheme for the arena allocators, dropping the distinction between bounded and unbounded.

* This patch also contains several javadoc improvements. I've considerably rewritten `VaList`, and also `ResourceScope` and `SegmentAllocator`, both to make sure that the latest changes in the API were reflected accordingly, but also to clarify certain aspects of the API.

[1] - https://git.openjdk.java.net/panama-foreign/pull/588
[2] - https://git.openjdk.java.net/panama-foreign/pull/509
[3] - http://cr.openjdk.java.net/~mcimadamore/panama/allocator_cleanup/javadoc/jdk/incubator/foreign/package-summary.html

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryHandles.java line 181:

> 179:      * the case if modeled as a Java {@code short}. This is illustrated in the following example:
> 180:      * <blockquote><pre>{@code
> 181:     MemorySegment segment = MemorySegment.allocateNative(2, ResourceScope.newImplicitScope());

Most changes in this patch are like this one: since `newConfinedScope()` is no longer automatically assigned a cleaner, we have to revert those changes back to what they were previously, by using an implicit scope instead. The difference compared to 17 is that the implicit scope is just a shared scope associated with a cleaner - it can be closed. There's no such concept of "special scope that can only be closed implicitly".

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

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