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.

Sign up for GitHub

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

Jump to bottom

8310355: Move the stub test from initialize_final_stubs() to test/hotspot/gtest #14765

Closed
wants to merge 1 commit into from
Closed

8310355: Move the stub test from initialize_final_stubs() to test/hotspot/gtest #14765

wants to merge 1 commit into from

Conversation

shqking
Copy link
Contributor

@shqking shqking commented Jul 4, 2023 •
edited by openjdk bot
Loading

Three groups of runtime routines, i.e. arraycopy, copy and fill, are tested inside function initialize_final_stubs(). The test runs every time the debug VM is started.

I think it's a usual convention that it's better not to run functional tests on startup. Hence, this patch proposes to move the stub test under test/hotspot/gtest.

It's one copy-paste patch, except the following two minor changes:

  1. Remove ASSERT condition check, and the gtest case will be run for release build as well.

  2. Use the gtest helper ASSERT_TRUE() to replace the assert().

Note that the downside is that we won't catch stub implementation errors immediately on startup.

Test:

  1. Cross compilations on arm32/s390/ppc/riscv passed.

  2. tier1 test passed on Linux/AArch64, Linux/x86_64 and macOS/Apple silicon. Note that hotspot/gtest is run in tier1.

  3. VM builds with several different options (e.g., zero build, release build, fastdebug build, client variant, no C1/c2) passed on Linux/AArch64 and Linux/x86_64.

  4. I manually injected errors in arraycopy/copy/fill stubs and verified make test TEST="gtest:StubRoutines" fails or not. But this check only worked for array fill stub. For the remaining two stubs, VM build (make images) failed firstly before running the gtest, because the built VM with these "erroneous" stubs were executed to do something such as loading Java core library code.

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

Issue

  • JDK-8310355: Move the stub test from initialize_final_stubs() to test/hotspot/gtest (Enhancement - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14765

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

Using diff file

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

Webrev

Link to Webrev Comment

@shqking
8310355: Move the stub test from initialize_final_stubs() to test/hot…
24c33ae 
…spot/gtest

Three groups of runtime routines, i.e. arraycopy, copy and fill, are
tested inside function `initialize_final_stubs()`. The test runs every
time the debug VM is started.

I think it's a usual convention that it's better not to run functional
tests on startup. Hence, this patch proposes to move the stub test under
`test/hotspot/gtest`.

It's one copy-paste patch, except the following two minor changes:

1) Remove `ASSERT` condition check, and the gtest case will be run for
release build as well.

2) Use the gtest helper `ASSERT_TRUE()` to replace the `assert()`.

Note that the downside is that we won't catch stub implementation errors
immediately on startup.

Test:

1) Cross compilations on arm32/s390/ppc/riscv passed.

2) tier1 test passed on Linux/AArch64, Linux/x86_64 and macOS/Apple
silicon. Note that hotspot/gtest is run in tier1.

3) VM builds with several different options (e.g., zero build, release
build, fastdebug build, client variant, no C1/c2) passed on
Linux/AArch64 and Linux/x86_64.

4) I manually injected errors in arraycopy/copy/fill stubs and verified
`make test TEST="gtest:StubRoutines"` fails or not. But this check only
worked for array fill stub. For the remaining two stubs, VM build (`make
images`) failed firstly before running the gtest, because the built VM
with these "erroneous" stubs were executed to do something such as
loading Java core library code.
@bridgekeeper
Copy link

bridgekeeper bot commented Jul 4, 2023

👋 Welcome back haosun! 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 openjdk bot added the rfr Pull request is ready for review label Jul 4, 2023
@openjdk
Copy link

openjdk bot commented Jul 4, 2023

@shqking The following label will be automatically applied to this pull request:

  • hotspot

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

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Jul 4, 2023
shqking
shqking commented Jul 4, 2023
View reviewed changes
test/hotspot/gtest/runtime/test_stubRoutines.cpp
@@ -0,0 +1,153 @@
/*
* Copyright (c) 1997, 2023, Oracle and/or its affiliates. All rights reserved.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of this file is copied from stubRoutines.cpp, hence I continue to use the copyright year from stubRoutines.cpp.

But I'm not 100% sure if it is correct. Please let me know if it is not. Thanks.

@mlbridge
Copy link

mlbridge bot commented Jul 4, 2023

Webrevs

@shqking
Copy link
Contributor Author

shqking commented Jul 10, 2023

Ping? Can anyone help take a look at this patch?
Thanks in advance.

@mseledts
Copy link
Member

Changes look good to me. Thank you for doing this change.

@openjdk
Copy link

openjdk bot commented Jul 18, 2023

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

8310355: Move the stub test from initialize_final_stubs() to test/hotspot/gtest

Reviewed-by: mseledtsov, lmesnik

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

  • 201e3bc: 8291065: Creating a VarHandle for a static field triggers class initialization
  • a53345a: 8287061: Support for rematerializing scalar replaced objects participating in allocation merges
  • 3236ba0: 8312189: ProblemList serviceability/jvmti/vthread/VThreadTLSTest/VThreadTLSTest.java#id1
  • 5cc71f8: 8312089: Simplify and modernize equals, hashCode, and compareTo in java.nio and implementation code
  • 6a09992: 8311968: Clarify Three-letter time zone IDs in java.util.TimeZone
  • 6ae152d: 8312196: ProblemList test/hotspot/jtreg/applications/ctw/modules/jdk_crypto_ec.java
  • 295ae11: 8312195: Changes in JDK-8284493 use wrong copyright syntax
  • f975be4: 8284493: Improve computeNextExponential tail performance and accuracy
  • e737968: 8308398: Move SunEC crypto provider into java.base
  • 69a46c2: 8310157: Allow void-returning filters for MethodHandles::collectCoordinates
  • ... and 107 more: https://git.openjdk.org/jdk/compare/60544f9088c11e4718a9cd77f21792c6ba387440...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 Jul 18, 2023
@shqking
Copy link
Contributor Author

shqking commented Jul 18, 2023

Thanks for your reviews! @mseledts and @lmesnik
The GHA tests are clean. Let me integrate it.

/integrate

@openjdk
Copy link

openjdk bot commented Jul 18, 2023

Going to push as commit 4b9ec82.
Since your change was applied there have been 119 commits pushed to the master branch:

  • 8ec136e: 8312072: Deprecate for removal the -Xnoagent option
  • fbe51e3: 8312127: FileDescriptor.sync should temporarily increase parallelism
  • 201e3bc: 8291065: Creating a VarHandle for a static field triggers class initialization
  • a53345a: 8287061: Support for rematerializing scalar replaced objects participating in allocation merges
  • 3236ba0: 8312189: ProblemList serviceability/jvmti/vthread/VThreadTLSTest/VThreadTLSTest.java#id1
  • 5cc71f8: 8312089: Simplify and modernize equals, hashCode, and compareTo in java.nio and implementation code
  • 6a09992: 8311968: Clarify Three-letter time zone IDs in java.util.TimeZone
  • 6ae152d: 8312196: ProblemList test/hotspot/jtreg/applications/ctw/modules/jdk_crypto_ec.java
  • 295ae11: 8312195: Changes in JDK-8284493 use wrong copyright syntax
  • f975be4: 8284493: Improve computeNextExponential tail performance and accuracy
  • ... and 109 more: https://git.openjdk.org/jdk/compare/60544f9088c11e4718a9cd77f21792c6ba387440...master

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Jul 18, 2023

@shqking Pushed as commit 4b9ec82.

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

@shqking shqking deleted the jdk-8310355 branch July 18, 2023 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lmesnik lmesnik lmesnik approved these changes

@mseledts mseledts mseledts approved these changes

Assignees
No one assigned
Labels
hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shqking @mseledts @lmesnik