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

8293114: JVM should trim the native heap #1616

Closed
wants to merge 5 commits into from

Conversation

shipilev
Copy link
Member

@shipilev shipilev commented Jul 21, 2023

Unclean backport to get useful experimental feature to LTS users.

In addition to usual contextual differences, there is a number of differences against upstream version:

  • synchronizer.cpp hunks are different, since we do not have JDK-8256302 in JDK 17
  • NativeHeapTrimmer_lock changed from Mutex::safepoint to Mutex::leaf, since JDK-8273915 renamed it after JDK 17
  • NativeHeapTrimmer_lock should also allow VMThread block and never require safepoint checks; this was simplified by JDK-8274004 later
  • vmError.cpp code was rewritten in JDK 17 style
  • gtest uses EXPECT_NOT_NULL, only brought by JDK-8295865 -- I rewrote them to EXPECT_NE/EQ.

Additional testing:

  • Linux x86_64 fastdebug, trim tests
  • Linux x86_64 fastdebug, tier1
  • Linux x86_64 fastdebug, tier2
  • Linux x86_64 fastdebug, tier3

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-8293114: JVM should trim the native heap (Enhancement - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-dev.git pull/1616/head:pull/1616
$ git checkout pull/1616

Update a local copy of the PR:
$ git checkout pull/1616
$ git pull https://git.openjdk.org/jdk17u-dev.git pull/1616/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1616

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk17u-dev/pull/1616.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 21, 2023

👋 Welcome back shade! A progress list of the required criteria for merging this PR into pr/1615 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 changed the title Backport 9e4fc568a6f1a93c84a84d6cc5220c6eb4e546a5 8293114: JVM should trim the native heap Jul 21, 2023
@openjdk
Copy link

openjdk bot commented Jul 21, 2023

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added the backport label Jul 21, 2023
@shipilev shipilev marked this pull request as ready for review July 21, 2023 17:57
@openjdk openjdk bot added the rfr Pull request is ready for review label Jul 21, 2023
@mlbridge
Copy link

mlbridge bot commented Jul 21, 2023

Webrevs

@openjdk
Copy link

openjdk bot commented Jul 25, 2023

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

@shipilev shipilev changed the base branch from pr/1615 to master July 25, 2023 14:19
@openjdk
Copy link

openjdk bot commented Jul 25, 2023

@shipilev Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

@shipilev
Copy link
Member Author

@tstuefe, please review if you have time.

Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

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

Good! Thanks for doing this, and sorry for the delay, I'm swamped.

I would wait a bit though, and let the patch cook upstream, before pushing. It will obviously also need https://bugs.openjdk.org/browse/JDK-8312525.

@openjdk
Copy link

openjdk bot commented Jul 31, 2023

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

8293114: JVM should trim the native heap

Reviewed-by: stuefe

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 31, 2023
@shipilev
Copy link
Member Author

shipilev commented Aug 14, 2023

I think we are ready for 17u integration. Do you agree, @tstuefe?

@tstuefe
Copy link
Member

tstuefe commented Aug 15, 2023

Hold up , I'll ask my SAP collegues first to put this through their backport CI.

@shipilev
Copy link
Member Author

Hold up , I'll ask my SAP collegues first to put this through their backport CI.

I see jdk17u-fix-yes from Goetz set today. Have you heard about testing results?

@tstuefe
Copy link
Member

tstuefe commented Aug 16, 2023

Hold up , I'll ask my SAP collegues first to put this through their backport CI.

I see jdk17u-fix-yes from Goetz set today. Have you heard about testing results?

Martin reported some errors. I suspect he sees the known ones that had been fixed with 8312525, but I need to confirm.

@tstuefe
Copy link
Member

tstuefe commented Aug 16, 2023

Okay, we see still problems in head at SAP. https://bugs.openjdk.org/browse/JDK-8314426. Lets wait with the downport till that one is fixed. Should be quick, hopefully.

@TheRealMDoerr
Copy link
Contributor

The backport looks good, but the test has issues in head as well. Thanks, Thomas, for investigating!

@shipilev
Copy link
Member Author

No problem, we can wait for that test to be fixed, and the backport for it lined up for 17u-dev, before we proceed with this integration. Then we would not break anyone's CI, hopefully.

@shipilev
Copy link
Member Author

Okay, we see still problems in head at SAP. https://bugs.openjdk.org/browse/JDK-8314426. Lets wait with the downport till that one is fixed. Should be quick, hopefully.

OK, I lined up the backport to 17u-dev for that fix here: #1687. Assuming that passes GHA testing, are we good with integrating all three PRs in 17u-dev? I would like to reserve some time to deal with any other low-probability fallout before 17.0.9 stabilization form.

@tstuefe
Copy link
Member

tstuefe commented Aug 23, 2023

Okay, we see still problems in head at SAP. https://bugs.openjdk.org/browse/JDK-8314426. Lets wait with the downport till that one is fixed. Should be quick, hopefully.

OK, I lined up the backport to 17u-dev for that fix here: #1687. Assuming that passes GHA testing, are we good with integrating all three PRs in 17u-dev? I would like to reserve some time to deal with any other low-probability fallout before 17.0.9 stabilization form.

I'm fine with it, but note that I will be gone throughout September.

@tstuefe
Copy link
Member

tstuefe commented Aug 25, 2023

I see you have all follow-ups already neatly aligned; will you ship it today or so to still get into the rapidly closing window?

@shipilev
Copy link
Member Author

I see you have all follow-ups already neatly aligned; will you ship it today or so to still get into the rapidly closing window?

Yes, my plan is to integrate today. I was checking if GHAs are still good with this PR.

@shipilev
Copy link
Member Author

Testing is clean. Review is here. Push approval granted.

/integrate

@openjdk
Copy link

openjdk bot commented Aug 25, 2023

Going to push as commit f667b35.
Since your change was applied there has been 1 commit pushed to the master branch:

  • 1421540: 8276651: java/lang/ProcessHandle tests fail with "RuntimeException: Input/output error" in java.lang.ProcessHandleImpl$Info.info0

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Aug 25, 2023

@shipilev Pushed as commit f667b35.

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

@shipilev shipilev deleted the JDK-8293114-trim-native branch December 11, 2023 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport integrated Pull request has been integrated
3 participants