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

8295044: Implementation of Foreign Function and Memory API (Second Preview) #10872

Closed
wants to merge 71 commits into from

Conversation

mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented Oct 26, 2022

This PR contains the API and implementation changes for JEP-434 [1]. A more detailed description of such changes, to avoid repetitions during the review process, is included as a separate comment.

[1] - https://openjdk.org/jeps/434


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change requires a CSR request to be approved

Issues

  • JDK-8295044: Implementation of Foreign Function and Memory API (Second Preview)
  • JDK-8295045: Implementation of Foreign Function and Memory API (Second Preview) (CSR)

Reviewers

Contributors

  • Jorn Vernee <jvernee@openjdk.org>
  • Per Minborg <pminborg@openjdk.org>
  • Maurizio Cimadamore <mcimadamore@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10872/head:pull/10872
$ git checkout pull/10872

Update a local copy of the PR:
$ git checkout pull/10872
$ git pull https://git.openjdk.org/jdk pull/10872/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10872

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10872.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 26, 2022

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

@openjdk
Copy link

openjdk bot commented Oct 26, 2022

⚠️ @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).

@openjdk
Copy link

openjdk bot commented Oct 26, 2022

@mcimadamore The following labels will be automatically applied to this pull request:

  • core-libs
  • hotspot
  • nio
  • security
  • serviceability
  • shenandoah

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added security security-dev@openjdk.org serviceability serviceability-dev@openjdk.org nio nio-dev@openjdk.org hotspot hotspot-dev@openjdk.org core-libs core-libs-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Oct 26, 2022
@mcimadamore
Copy link
Contributor Author

Here are the main API changes introduced in this round (there are also some JVM changes which will be integrated separately):

  • The main change is the removal of MemoryAddress and Addressable. Instead, zero-length memory segments are used whenever the API needs to model "raw" addresses coming from native code. This simplifies the API, removing an ambiguous abstraction as well as some duplication in the API (see accessor methods in MemoryAddress);

  • To allow for "unsafe" access of zero-length memory segments, a new method has been added to ValueLayout.OfAddress, namely asUnbounded. This new restricted method takes an address layout and creates a new unbounded address layout. When using an unbounded layout to dereference memory, or construct downcall method handles, the API will create memory segments with maximal length (i.e. Long.MAX_VALUE, rather than zero-length memory segments, which can therefore be accessed;

  • The MemoryLayout hierarchy has been improved in several ways. First, the hierarchy is now defined in terms of sealed interfaces (intermediate abstract classes have been moved into the implementation package). The hierarchy is also exhaustive now, and works much better to pattern matching. More specifically, three new types have been added: PaddingLayout, StructLayout and UnionLayout, the latter two are a subtype of GroupLayout. Thanks to this move, several predicate methods (isPadding, isStruct, isUnion) have been dropped from the API;

  • The SymbolLookup::lookup method has been renamed to SymbolLookup::find - to avoid using the same word lookup in both noun and verb form, which leads to confusion;

  • A new method, on ModuleLayer.Controller has been added to enable native access on a module in a custom layer;

  • The new interface Linker.Option has been introduced. This is a tag interface accepted in Linker::downcallHandle. At the moment, only a single option is provided, to specify variadic function calls (because of this, the FunctionDescriptor interface has been simplified, and is now a simple carrier of arguments/return layouts). More linker options will follow.

@wangweij
Copy link
Contributor

/label remove security

@openjdk openjdk bot removed the security security-dev@openjdk.org label Oct 27, 2022
@openjdk
Copy link

openjdk bot commented Oct 27, 2022

@wangweij
The security label was successfully removed.

@mcimadamore mcimadamore marked this pull request as ready for review October 27, 2022 20:53
@openjdk
Copy link

openjdk bot commented Nov 30, 2022

@mcimadamore this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout PR_20
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Nov 30, 2022
@openjdk openjdk bot removed merge-conflict Pull request has merge conflict with target branch csr Pull request needs approved CSR before integration labels Nov 30, 2022
@openjdk
Copy link

openjdk bot commented Dec 4, 2022

@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:

8295044: Implementation of Foreign Function and Memory API (Second Preview)

Co-authored-by: Jorn Vernee <jvernee@openjdk.org>
Co-authored-by: Per Minborg <pminborg@openjdk.org>
Co-authored-by: Maurizio Cimadamore <mcimadamore@openjdk.org>
Reviewed-by: jvernee, pminborg, psandoz, alanb, sundar

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 67 new commits pushed to the master branch:

  • d523d9d: 8297864: Dead code elimination
  • 777fb52: 8297974: ClassCastException in com.sun.tools.javac.comp.AttrRecover.doRecovery
  • 17666fb: 8297794: Deprecate JMX Management Applets for Removal
  • 619b68c: 8294540: Remove Opaque2Node: it is broken and triggers assert
  • 82561de: 8296384: [TESTBUG] sun/security/provider/SecureRandom/AbstractDrbg/SpecTest.java intermittently timeout
  • 61b7093: 8297872: Non-local G1MonotonicArenaFreePool::_freelist_pool has non-trivial ctor/dtor
  • 3b3bbe5: 8296907: VMError: add optional callstacks, siginfo for secondary errors
  • a573923: 8297264: C2: Cast node is not processed again in CCP and keeps a wrong too narrow type which is later replaced by top
  • b49fd92: 8298055: AArch64: fastdebug build fails after JDK-8247645
  • 914ef07: 8297609: Add application/wasm MIME type for wasm file extension
  • ... and 57 more: https://git.openjdk.org/jdk/compare/4485d4e517b6dece7a9eeb5cf9a2180d84956da3...master

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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Dec 4, 2022
@mcimadamore
Copy link
Contributor Author

/contributor add @JornVernee @minborg @mcimadamore

@openjdk
Copy link

openjdk bot commented Dec 5, 2022

@mcimadamore @JornVernee @minborg @mcimadamore is not a valid user in this repository.

Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <duke@openjdk.org>

User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address.

@mcimadamore
Copy link
Contributor Author

/contributor add @JornVernee

@mcimadamore
Copy link
Contributor Author

/contributor add @minborg

@mcimadamore
Copy link
Contributor Author

/contributor add @mcimadamore

@openjdk
Copy link

openjdk bot commented Dec 5, 2022

@mcimadamore
Contributor Jorn Vernee <jvernee@openjdk.org> successfully added.

@openjdk
Copy link

openjdk bot commented Dec 5, 2022

@mcimadamore
Contributor Per Minborg <pminborg@openjdk.org> successfully added.

@openjdk
Copy link

openjdk bot commented Dec 5, 2022

@mcimadamore
Contributor Maurizio Cimadamore <mcimadamore@openjdk.org> successfully added.

Copy link
Member

@sundararajana sundararajana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcimadamore
Copy link
Contributor Author

mcimadamore commented Dec 5, 2022

Note: there are 4 tests failing in x86:

  • MemoryLayoutPrincipalTotalityTest
  • MemoryLayoutTypeRetentionTest
  • TestLargeSegmentCopy
  • TestLinker

These failures are addressed in the dependent PR: https://git.openjdk.org/jdk/pull/11019, which will be integrated immediately after these changes

@mcimadamore
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Dec 5, 2022

Going to push as commit 73baadc.
Since your change was applied there have been 74 commits pushed to the master branch:

  • bd38188: 8297766: Remove UseMallocOnly development option
  • b9eec96: 8281214: Unsafe use of long in VMThread::setup_periodic_safepoint_if_needed
  • e7e0354: 8297767: Assert JNICritical_lock/safepoint-1 and AdapterHandlerLibrary_lock/safepoint-1
  • f9e0f1d: 8297763: Fix missing stub code expansion before align() in shared trampolines
  • 2300ed4: 8291769: Translation of switch with record patterns could be improved
  • eab0ada: 8296545: C2 Blackholes should allow load optimizations
  • dea2161: 8297959: Provide better descriptions for some Operating System JFR events
  • d523d9d: 8297864: Dead code elimination
  • 777fb52: 8297974: ClassCastException in com.sun.tools.javac.comp.AttrRecover.doRecovery
  • 17666fb: 8297794: Deprecate JMX Management Applets for Removal
  • ... and 64 more: https://git.openjdk.org/jdk/compare/4485d4e517b6dece7a9eeb5cf9a2180d84956da3...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Dec 5, 2022
@openjdk openjdk bot closed this Dec 5, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 5, 2022
@openjdk
Copy link

openjdk bot commented Dec 5, 2022

@mcimadamore Pushed as commit 73baadc.

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

@brcolow
Copy link

brcolow commented Dec 14, 2022

@mcimadamore This PR made my code in java-vulkan much cleaner! Nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated nio nio-dev@openjdk.org serviceability serviceability-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org
10 participants