Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8273905: Foreign API refresh #576

Closed

Conversation

mcimadamore
Copy link
Collaborator

@mcimadamore mcimadamore commented Sep 16, 2021

As outlined in [1], there are areas of the foreign API we'd like to improve upon, such as:

  • dereference: there is a mismatch between API points which create segments (which take a layout) and dereference API points, which do not take layouts

  • role of MemoryAddress: in Java 17, MemoryAddress has become a stateful carrier, which is attached to a scope. This is inconvenient, as in most cases, a memory address is just a raw pointer that arises when interacting with native code.

  • resource scopes: the API for scopes has too many flavors to pick from, many of which overlap. The fact that scopes and segment allocators are unrelated forces clients to introduce ad hoc conversions from scopes to allocators, and library developers to add overloads. Finally, the API for acquiring scopes doesn't work well with try-with-resources, and could also be simplified.

I will add separate comments to explain how the API has changed to resolve the above issues.

[1] - https://mail.openjdk.java.net/pipermail/panama-dev/2021-September/014946.html


Progress

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

Issue

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 576

View PR using the GUI difftool:
$ git pr show -t 576

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/panama-foreign/pull/576.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 16, 2021

👋 Welcome back mcimadamore! A progress list of the required criteria for merging this PR into foreign-memaccess+abi will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@mcimadamore mcimadamore changed the title Changes to finalize the foreign memory access/linker APIs. 8273905: Foreign API refresh Sep 16, 2021
@mcimadamore
Copy link
Collaborator Author

mcimadamore commented Sep 16, 2021

Disclaimer: the proposed changes touch several areas of the APIs (in deep ways) which makes it hard (if not impossible) to break the changes apart into multiple PRs. Below is an attempt to describe the main areas affected by this PR.

Memory dereference

The main change in this area is to attach carriers to ValueLayout. Because of this, we have defined several subclasses of ValueLayout (one per carrier), such as ValueLayout.OfInt, ValueLayout.OfFloat and so forth. Attaching layouts to carriers allows expressing deference method elegantly - e.g. instead of:

MemoryAccess.getIntAtOffset(segment, offset)

simply:

segment.get(JAVA_INT, offset);

The new dereference methods are statically safe: it is not possible for clients to pass values to setters of the wrong type. We have used the same trick in different parts of the API, to make it more uniform and safer (see changes in SegmentAllocator and VaList). As a result, MemoryAccess and MemoryCopy go away, as the same operations are now available as instance (or static, in the case of copy) methods in MemorySegment. Since the new dereference API points are parameterized by a layout, it is always possible to e.g. create a layout with desired endianness and use that instead. The only caveat is that, for performance to be optimal, the layout using during dereference should be a VM constant (e.g. static final field).

CLinker

First, we have moved most of the CLinker helper methods away from CLinker. For instance, to allocate an Utf8 string, there is now a method in SegmentAllocator, as well as a pair of getter/setter in MemorySegment. This makes the code easier to follow.
Since carriers are attached to value layout, passing a MethodType to CLinker::downcallHandle is no longer necessary, so the API for obtaining downcall method handles has been simplified.

Some things have been removed as well; most notably, the platform-dependent layout constants, such as C_INT are gone. This is because we cannot type these constants sharply - given that their type would vary across platforms (is C_LONG a ValueLayout.OfInt or a ValueLayout.OfLong ?). Ultimately, it is the role of a tool (such as jextract) to generate a set of layout constants which work on a given platform, so we have made the API more neutral. It is still possible to use layouts such as JAVA_INT and JAVA_LONG to link downcall method handles, and that is not too different from using JNI types such as jint and jlong (that is, they should be used with care, making sure they match the underlying platform types).

Another thing we removed was the freeMemory and allocateMemory API points. These were just shortcuts for commonly used downcall method handles, which in most of the cases will be included in a jextraction unit anyway. There seems to be little value in including such methods in the CLinker API.

Layouts

The main change to memory layouts is the removal of the layout attribute mechanism. This mechanism was powerful, but also very prone to misuse; it was unfortunate that the whole linker classification story depended on it as well, as that meant that linker-related attributes were mixed in with user-defined ones. If needed, we might well re-introduce some form of user-defined attribute in the future (e.g. a Map which can be attached to a layout, which will be preserved by the immutable withers). But for now we would like to keep the API minimal, as we think that (bar few exceptions) most of the use cases for the attribute API were internal. It is also possible that now that layouts are associated with carriers, some of the things previously stored in attributes might be stored in a ClassValue instead.

A minor change/simplification to the layout API is that obtaining a memory access var handle from a layout no longer requires a Class parameter, as that parameter is now inferred from the layout. Internally, all value layouts now keep a @Stable var handle cached field, so that dereferencing through them can happen at optimal speed.

Memory address

MemoryAddress goes back to being a dumb wrapper around a long. In fact, we have dropped support for heap-based MemoryAddress and made MemoryAddress a native-only concept, since addresses are only really used when interacting with native code (in all other use cases, using a segment is the best option).

You will notice that the Addressable interface has been enhanced slightly, to include a scope. This allows us to open up the Addressable API, so that clients can define custom Addressable subclasses, and so that the CLinker would be able to keep addressable parameters alive in the context of a native call. In fact, the linker no longer erases address layouts down to MemoryAddress, but it erases them to Addressable instead, so that the scope is preserved.

MemoryAddress is now an implementation of Addressable whose scope is always the global scope.

Resource scopes

We decided to drop the concept of implicit scopes. After all, we already had the notion of confined/shared explicit scopes which were also backed by a cleaner. This is sufficient to cover the implicit cases too.

Another simplification we have done is in the acquire/release mechanism; you will notice that there's only one method now, called keepAlive, which takes a target scope. This method will essentially acquire the target scope, which will only be released when the receiver scope is closed.

Finally, we have tweaked the ResourceScope interface to extend SegmentAllocator. There are many instances where an allocator is required where a client only has a scope. This subtyping allows clients to get rid of redundant conversion (e.g. SegmentAllocator.ofScope(scope)) and just pass a scope where an allocator is expected. Combined with other changes in the dereference API, we believe this makes the API a lot more fluent.

@mcimadamore mcimadamore marked this pull request as ready for review Sep 16, 2021
@openjdk openjdk bot added the rfr Ready for review label Sep 16, 2021
@mlbridge
Copy link

mlbridge bot commented Sep 16, 2021

@PaulSandoz
Copy link
Member

PaulSandoz commented Sep 17, 2021

I recommend running IntelliJ analysis over the code before integrating. It often finds small problems, typos, Java doc issues.

@mcimadamore
Copy link
Collaborator Author

mcimadamore commented Sep 17, 2021

I recommend running IntelliJ analysis over the code before integrating. It often finds small problems, typos, Java doc issues.

I've ran the typos and grammar analyses already (so what you see should be already relatively polished). I will run some more to catch code issues.

mcimadamore added 2 commits Sep 17, 2021
* unused imports
* suggestions for new language features
* drop unused impl declarations
* removed size constructor parameter in value layout subclasses (except `OfAddress`)
Copy link
Member

@uschindler uschindler left a comment

I found some minor javadoc things (just by looking at it and searching for defintiions, so there may be other problems).

I did some testing in Apache Lucene with a build of your branch: https://github.com/uschindler/lucene/tree/draft/jdk-foreign-mmap-jdk18 (the relevant change to previous version using the old MemoryCopy stuff is this commit: uschindler/lucene@2de6506

The new API looks really good. I also removed all usage of VarHandle in the Lucene implementation and just used the versions of MemorySegment#get(ValueLayout.Xxx,...) for our positional reads. Interestingly the performance seemed to get a bit better without direct use of VarHandles (maybe because you use @ForceInline), but not significant (inside the benchmark's std dev).

The most interesting code for you is here: https://github.com/uschindler/lucene/blob/draft/jdk-foreign-mmap-jdk18/lucene/core/src/java/org/apache/lucene/store/MemorySegmentIndexInput.java

mcimadamore added 2 commits Sep 20, 2021
* Fix spurious javadoc references to `MemoryLayouts`
* Drop one more unused import
* Add checks to `MemorySegment::copy`: array component should match layout carrier
* Tweak exception thrown by `MemorySegment::copy` when an unsupported array is passed
* Add simple negative tests to `TestArrayCopy`
mcimadamore added 4 commits Sep 20, 2021
* Fix javadoc of `UpcallStub` so that summary does not contain link.
* Add `@throws IllegalArgumentException` on `ResourceScope::keepAlive`
* Add `@throws IllegalStateException` on `ResourceScope::keepAlive`
* Add resource scope test to check that keeping alive self is not allowed
…umber of keep alive requests

* Make sure that the keep alive limit is also enforced in confined scope implementations
@mlbridge
Copy link

mlbridge bot commented Sep 20, 2021

Mailing list message from Maurizio Cimadamore on panama-dev:

Many thanks for testing this and confirming that there are no regressions!

Maurizio

On 18/09/2021 22:11, Uwe Schindler wrote:

I did some testing in Apache Lucene with a build of your branch:https://github.com/uschindler/lucene/tree/draft/jdk-foreign-mmap-jdk18 (the relevant change to previous version using the old MemoryCopy stuff is this commit:https://github.com/uschindler/lucene/commit/2de65069a7419edd43f5fc3b63e6e9fa077e2235

The new API looks really good. I also removed all usage of `VarHandle` in the Lucene implementation and just used the versions of `MemorySegment#get(ValueLayout.Xxx,...)` for our positional reads. Interestingly the performance seemed to get a bit better without direct use of VarHandles (maybe because you use `@ForceInline`), but not significant (inside the benchmark's std dev).

The most interesting code for you is here:https://github.com/uschindler/lucene/blob/draft/jdk-foreign-mmap-jdk18/lucene/core/src/java/org/apache/lucene/store/MemorySegmentIndexInput.java

Copy link
Member

@PaulSandoz PaulSandoz left a comment

Looks good, just one further issue identified with the upcall stub implementation, which I believe should be easy to fix.

@openjdk
Copy link

openjdk bot commented Sep 21, 2021

@mcimadamore This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8273905: Foreign API refresh

Reviewed-by: uschindler, psandoz

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 88 new commits pushed to the foreign-memaccess+abi branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the foreign-memaccess+abi branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Ready to be integrated label Sep 21, 2021
* Beefed up SafeFunctionAccessTest
@mcimadamore
Copy link
Collaborator Author

mcimadamore commented Sep 22, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Sep 22, 2021

Going to push as commit 831e75b.
Since your change was applied there have been 88 commits pushed to the foreign-memaccess+abi branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Sep 22, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review labels Sep 22, 2021
@openjdk
Copy link

openjdk bot commented Sep 22, 2021

@mcimadamore Pushed as commit 831e75b.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
4 participants