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

8277845: Clean up use if ProgrammableInvoker/UpcallHandler.USE_INTRINSICS in tests. #629

Conversation

JornVernee
Copy link
Member

@JornVernee JornVernee commented Jan 10, 2022

This PR cleans up use of USE_INTRINSICS flags in the tests, which was removed a while ago.

I also noticed that TestMatrix.java didn't have a copyright header, so I've added one. (the diff looks a little mangled as a result). It also wasn't setting the UPCALL_TEST_TYPE flag when running TestUpcall, so I've added that flag.


Progress

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

Issue

  • JDK-8277845: Clean up use if ProgrammableInvoker/UpcallHandler.USE_INTRINSICS in tests.

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 629

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 10, 2022

👋 Welcome back jvernee! 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.

@openjdk openjdk bot added the rfr label Jan 10, 2022
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 10, 2022

@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Jan 10, 2022

It also wasn't setting the UPCALL_TEST_TYPE flag when running TestUpcall, so I've added that flag.

Actually, I think now might be a good time to refactor this as well, by splitting the different upcall test types into base classes instead of using a flag to select each one, which also avoids the console log being full of SkipException stack traces.

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Changes look good. I totally agree re. refactoring upcall tests - either as part of this PR, or a separate one.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 10, 2022

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

8277845: Clean up use if ProgrammableInvoker/UpcallHandler.USE_INTRINSICS in tests.

Reviewed-by: mcimadamore

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 85 new commits pushed to the foreign-memaccess+abi branch:

  • 6730407: Automatic merge of master into foreign-memaccess+abi
  • b4951ef: Automatic merge of jdk:master into master
  • 84976b4: 8278549: UNIX sun/font coding misses SUSE distro detection on recent distro SUSE 15
  • 965c64b: 8279699: Parallel: More precise boundary in ObjectStartArray::object_starts_in_range
  • 35172cd: 8278951: containers/cgroup/PlainRead.java fails on Ubuntu 21.10
  • 237f861: 8273143: Transition to _thread_in_vm when handling a polling page exception
  • 9209e6d: 8279877: Document IDEA IDE setup in docs/ide.md
  • 0a839b4: 8279801: EC KeyFactory and KeyPairGenerator do not have aliases for OID format
  • 6fcaa32: 8262442: (windows) Use all proxy configuration sources when java.net.useSystemProxies=true
  • c17a012: 8278961: Enable debug logging in java/net/DatagramSocket/SendDatagramToBadAddress.java
  • ... and 75 more: https://git.openjdk.java.net/panama-foreign/compare/e0afc86aee18d229dc7446b37d721b5dfb638397...foreign-memaccess+abi

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 label Jan 10, 2022
@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Jan 10, 2022

I've implemented the refactor. (also double the timeout of UpcallAsync when running from TestMatrix, since it was timing out otherwise).

- Make static field only used in the base class private.
- Capitalize static field names.
@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Jan 18, 2022

Addressed in the latest commit:

  • Move loadLibrary calls to base classes.
  • Make static fields only used in the base class private.
  • Capitalize static field names.

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Looks good

@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Jan 18, 2022

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jan 18, 2022

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

  • 6730407: Automatic merge of master into foreign-memaccess+abi
  • b4951ef: Automatic merge of jdk:master into master
  • 84976b4: 8278549: UNIX sun/font coding misses SUSE distro detection on recent distro SUSE 15
  • 965c64b: 8279699: Parallel: More precise boundary in ObjectStartArray::object_starts_in_range
  • 35172cd: 8278951: containers/cgroup/PlainRead.java fails on Ubuntu 21.10
  • 237f861: 8273143: Transition to _thread_in_vm when handling a polling page exception
  • 9209e6d: 8279877: Document IDEA IDE setup in docs/ide.md
  • 0a839b4: 8279801: EC KeyFactory and KeyPairGenerator do not have aliases for OID format
  • 6fcaa32: 8262442: (windows) Use all proxy configuration sources when java.net.useSystemProxies=true
  • c17a012: 8278961: Enable debug logging in java/net/DatagramSocket/SendDatagramToBadAddress.java
  • ... and 75 more: https://git.openjdk.java.net/panama-foreign/compare/e0afc86aee18d229dc7446b37d721b5dfb638397...foreign-memaccess+abi

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label Jan 18, 2022
@openjdk openjdk bot closed this Jan 18, 2022
@openjdk openjdk bot removed ready rfr labels Jan 18, 2022
@openjdk
Copy link

@openjdk openjdk bot commented Jan 18, 2022

@JornVernee Pushed as commit 1e1574c.

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

@JornVernee JornVernee deleted the Cleanup_USE_INTRINSICS branch Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated
2 participants