jdk17u-dev icon indicating copy to clipboard operation
jdk17u-dev copied to clipboard

8315362: NMT: summary diff reports threads count incorrectly

Open zer0chance opened this issue 1 year ago • 8 comments
trafficstars


Progress

  • [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • [x] Change must not contain extraneous whitespace
  • [x] Commit message must refer to an issue
  • [ ] JDK-8315362 needs maintainer approval

Issue

  • JDK-8315362: NMT: summary diff reports threads count incorrectly (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2084

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

Using diff file

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

Webrev

Link to Webrev Comment

zer0chance avatar Dec 27 '23 07:12 zer0chance

:wave: Welcome back zer0chance! 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.

bridgekeeper[bot] avatar Dec 27 '23 07:12 bridgekeeper[bot]

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

openjdk[bot] avatar Dec 27 '23 07:12 openjdk[bot]

Webrevs

mlbridge[bot] avatar Dec 27 '23 10:12 mlbridge[bot]

@zer0chance This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Jan 24 '24 11:01 bridgekeeper[bot]

Adding comment to keep this PR alive. @GoeLin do I need to do anything else to get your approval on this?

zer0chance avatar Feb 13 '24 16:02 zer0chance

@zer0chance This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Mar 12 '24 17:03 bridgekeeper[bot]

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

8315362: NMT: summary diff reports threads count incorrectly

Reviewed-by: mdoerr

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TheRealMDoerr) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk[bot] avatar Mar 13 '24 19:03 openjdk[bot]

@zer0chance This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Apr 10 '24 21:04 bridgekeeper[bot]

@zer0chance This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

bridgekeeper[bot] avatar May 09 '24 00:05 bridgekeeper[bot]

/open

zer0chance avatar May 23 '24 09:05 zer0chance

@zer0chance This pull request is now open

openjdk[bot] avatar May 23 '24 09:05 openjdk[bot]

This PR shouldn't differ from the 21u version: https://github.com/openjdk/jdk21u/commit/31c759dd7ef69449e225c3e656d8f522e1124852 I currently see the following diffs:

< +  _thread_count ++;
---
> +  _thread_count++;
< +        output.shouldContain("Baseline succeeded");
---
> +        output.shouldContain("Baseline taken");

TheRealMDoerr avatar May 27 '24 13:05 TheRealMDoerr

This PR shouldn't differ from the 21u version: openjdk/jdk21u@31c759d I currently see the following diffs:

< +  _thread_count ++;
---
> +  _thread_count++;
< +        output.shouldContain("Baseline succeeded");
---
> +        output.shouldContain("Baseline taken");

For _thread_count is a good catch, but we need to modify the test like this because JDK-8289182 was not backported to the JDK 17 and jcmd VM.native_memory baseline outputs Baseline succeeded not Baseline taken

zer0chance avatar May 28 '24 11:05 zer0chance

⚠️ @zer0chance This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

openjdk[bot] avatar May 28 '24 11:05 openjdk[bot]

Should be adapted if https://github.com/openjdk/jdk17u-dev/pull/2415 gets approved and integrated.

TheRealMDoerr avatar May 28 '24 11:05 TheRealMDoerr

Thank you @TheRealMDoerr! Can I please ask you to take a look to the follow up fix backport https://github.com/openjdk/jdk17u-dev/pull/2085?

zer0chance avatar May 28 '24 12:05 zer0chance

/approval

zer0chance avatar May 28 '24 12:05 zer0chance

@zer0chance usage: /approval [<id>] (request|cancel) [<text>]

openjdk[bot] avatar May 28 '24 12:05 openjdk[bot]

/approval request

zer0chance avatar May 28 '24 12:05 zer0chance

@zer0chance 8315362: The approval request has been created successfully.

openjdk[bot] avatar May 28 '24 12:05 openjdk[bot]

https://github.com/openjdk/jdk17u-dev/pull/2085 is clean and trivial. It doesn't need a review. However, all backports need maintainer approvals (see https://wiki.openjdk.org/display/JDKUpdates/JDK+17u). We typically describe in the approval request the motivation for doing the backport, how risky it is and how we tested it. Having said this, please enable GitHub Actions to get automatic tests running on your PRs. I have requested approval for https://github.com/openjdk/jdk17u-dev/pull/2415, so that one should get integrated first and this one become a clean backport of the JDK 21 version.

TheRealMDoerr avatar May 28 '24 13:05 TheRealMDoerr

Both PRs got maintainer approval. Please change to "Baseline taken" once https://github.com/openjdk/jdk17u-dev/pull/2415 is integrated.

TheRealMDoerr avatar May 29 '24 10:05 TheRealMDoerr

@zer0chance 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/8315362-backport
git fetch https://git.openjdk.org/jdk17u-dev.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk[bot] avatar May 29 '24 14:05 openjdk[bot]

@TheRealMDoerr test is adjusted

zer0chance avatar May 29 '24 14:05 zer0chance

/integrate

zer0chance avatar May 29 '24 14:05 zer0chance

@zer0chance Your change (at version f647537911374745a217158eddaafa1cd3df73f3) is now ready to be sponsored by a Committer.

openjdk[bot] avatar May 29 '24 14:05 openjdk[bot]

Ok, you could have made it clean by naming it "Backport 31c759dd7ef69449e225c3e656d8f522e1124852" which makes it a backport of the JDK 21 version. But, let's ship it! /sponsor

TheRealMDoerr avatar May 31 '24 09:05 TheRealMDoerr

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

  • 9eb970264a34c82c11468b15fae89dc39c574682: 8331164: createJMHBundle.sh download jars fail when url needed to be redirected
  • 46ba9bbe7e93c561b5af833ff164596955302a9d: 8318479: [jmh] the test security.CacheBench failed for multiple threads run
  • 1de0c0a805f68914aec7358fc4aa9c9d2499ec98: 8256291: RunThese30M fails "assert(_class_unload ? true : ((((JfrTraceIdBits::load(class_loader_klass)) & ((1 << 4) << 8)) != 0))) failed: invariant"
  • eebb80c4bcec64dc70065b597688cbe12c892a5b: 8317288: [macos] java/awt/Window/Grab/GrabTest.java: Press on the outside area didn't cause ungrab
  • cc0c14380337f868d63f4ab746e4746a5dc2f115: 8310108: Skip ReplaceCriticalClassesForSubgraphs when EnableJVMCI is specified
  • 7a8cf6c8aec6cd648663def62d9496e516f05d4f: 8319713: Parallel: Remove PSAdaptiveSizePolicy::should_full_GC
  • 515bc9a264c1e44cfcada0405a74ab2fba428e10: 8318986: Improve GenericWaitBarrier performance
  • dcb2c35151a846b7c44045235b389baf9713885c: 8295111: dpkg appears to have problems resolving symbolically linked native libraries

Your commit was automatically rebased without conflicts.

openjdk[bot] avatar May 31 '24 09:05 openjdk[bot]

@TheRealMDoerr @zer0chance Pushed as commit a058ad151a7c68f65e915856b089ce4064c1caf8.

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

openjdk[bot] avatar May 31 '24 09:05 openjdk[bot]