8272164: DumpAllocStats shouldn't subclass from ResourceObj

[DanHeidinga]

I picked up a simple unassigned bug to work through the contribution process.

make run-test-tier1 passes with this change.

Signed-off-by: Dan Heidinga heidinga@redhat.com

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8272164: DumpAllocStats shouldn't subclass from ResourceObj

Reviewers

• Aleksey Shipilev (@shipilev - Reviewer) ⚠️ Review applies to e742c19c9fa431b0cc8b15708bdd46247631a631
• Ioi Lam (@iklam - Reviewer) ⚠️ Review applies to 8438b69
• David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5362/head:pull/5362
$ git checkout pull/5362

Update a local copy of the PR:
$ git checkout pull/5362
$ git pull https://git.openjdk.java.net/jdk pull/5362/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5362

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5362.diff

[bridgekeeper]

👋 Welcome back DanHeidinga! 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]

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

• hotspot-runtime

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.

[mlbridge]

Webrevs

• 02: Full - Incremental (b0781da0)
• 01: Full (8438b691)
• 00: Full (e742c19c)

[shipilev]

Looks fine to me. Please configure and run GHA testing: look at "Pre-submit test status" or "Checks" tab.

[openjdk]

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

8272164: DumpAllocStats shouldn't subclass from ResourceObj

Reviewed-by: shade, iklam, dholmes

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

• 1bf5bda: 8269418: jdk/jfr/event/oldobject/TestObjectSize.java failed with "RuntimeException: No events: expected false, was true"
• fb5b144: 8272985: Reference discovery is confused about atomicity and degree of parallelism
• 70ed6c5: 8272878: JEP 381 cleanup: Remove unused Solaris code in sun.font.TrueTypeGlyphMapper
• b4e5b28: 8273221: Guard GCIdMark against nested calls
• 4d25e6f: 8273335: compiler/blackhole tests should not run with interpreter-only VMs
• c640fe4: 7188098: TEST_BUG: closed/javax/sound/midi/Synthesizer/Receiver/bug6186488.java fails
• cec6c06: 8272232: javax/swing/JTable/4275046/bug4275046.java failed with "Expected value in the cell: 'rededited' but found 'redEDITED'."
• 14a3ac0: 8271911: replay compilations of methods which use JSR292 (easy cases)
• d414a88: 8273240: Dynamic test ArchiveConsistency.java should use CDSArchiveUtils

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

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 (@shipilev, @iklam, @dholmes-ora) 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).

[DanHeidinga]

I see the gc/stringdedup/TestStringDeduplicationAgeThreshold.java#id3 test failed on Linux_x64 and that there's a fix has already been merged for it:
7212561

I'll rebase to pick up the latest to get a clean test run

[iklam]

LGTM

[shipilev]

I'll rebase to pick up the latest to get a clean test run

You can, but no need to spend CPU time on this, as long as you know the test failure was not caused by this PR. GHA results are not required to be fully green for integration. Also, do merges not rebases, if you can -- leaves the reviews/webrevs consistent, especially when comments are bound to particular code locations.

[DanHeidinga]

Apologies - I've already pushed the rebase. Will use merges in the future

[dholmes-ora]

This class is never allocated from resource area or C-heap. It's just stack allocated.

So the obvious question - should it subclass StackObj instead?

[dholmes-ora]

Signed-off-by: Dan Heidinga heidinga@redhat.com

What does this signify Dan?

Cheers,
David

[DanHeidinga]

Signed-off-by: Dan Heidinga heidinga@redhat.com

What does this signify Dan?

I've gotten in the habit of using git commit -s to sign commits as this is required for contributing to Eclipse projects. The intent of the line is to show that the author is signing off that they have the rights to share the patch. It's usually used to ensure that the author of the pull request has signed the Eclipse Contributor agreement. I guess it's not used here?

[dholmes-ora]

I guess it's not used here?

Right, not used here. The patch author has to have signed the OCA for the PR to be accepted by the skara tooling.

Cheers,
David

[dholmes-ora]

Hi Dan,

This version also seems fine. I'm not sure where we stand with using StackObj these days in terms of style (ie is it just a historical anachronism?). But in this case DumpAllocStats only has a single use embedded within ArchiveBuilder which is itself a StackObj - so this seems fine either way.

Thanks,
David

[DanHeidinga]

The pre-submit test failures are all in tools/javac/options/modes/InfoOptsTest.java and I can't reproduce the failure locally. There a a number of other open PRs with the same failure so it looks like an unrelated failure.

[DanHeidinga]

/integrate

[openjdk]

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

[shipilev]

/sponsor

[openjdk]

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

• 1bf5bda: 8269418: jdk/jfr/event/oldobject/TestObjectSize.java failed with "RuntimeException: No events: expected false, was true"
• fb5b144: 8272985: Reference discovery is confused about atomicity and degree of parallelism
• 70ed6c5: 8272878: JEP 381 cleanup: Remove unused Solaris code in sun.font.TrueTypeGlyphMapper
• b4e5b28: 8273221: Guard GCIdMark against nested calls
• 4d25e6f: 8273335: compiler/blackhole tests should not run with interpreter-only VMs
• c640fe4: 7188098: TEST_BUG: closed/javax/sound/midi/Synthesizer/Receiver/bug6186488.java fails
• cec6c06: 8272232: javax/swing/JTable/4275046/bug4275046.java failed with "Expected value in the cell: 'rededited' but found 'redEDITED'."
• 14a3ac0: 8271911: replay compilations of methods which use JSR292 (easy cases)
• d414a88: 8273240: Dynamic test ArchiveConsistency.java should use CDSArchiveUtils

Your commit was automatically rebased without conflicts.

[openjdk]

@shipilev @DanHeidinga Pushed as commit 44c5c23.

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