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

8317692: jcmd GC.heap_dump performance regression after JDK-8292818 #16083

Closed
wants to merge 8 commits into from

Conversation

SirYwell
Copy link
Member

@SirYwell SirYwell commented Oct 6, 2023

See the bug description for more information.

This implementation brings down the time to take a heap dump on the example application in the bug report to <2 seconds on my machine.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

  • JDK-8317692: jcmd GC.heap_dump performance regression after JDK-8292818 (Bug - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16083

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 6, 2023

👋 Welcome back hgreule! 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 changed the title JDK-8317692 8317692: jcmd GC.heap_dump performance regression after JDK-8292818 Oct 6, 2023
@openjdk
Copy link

openjdk bot commented Oct 6, 2023

@SirYwell 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 Oct 6, 2023
@SirYwell
Copy link
Member Author

SirYwell commented Oct 6, 2023

/label add serviceability

@openjdk openjdk bot added rfr Pull request is ready for review serviceability serviceability-dev@openjdk.org labels Oct 6, 2023
@openjdk
Copy link

openjdk bot commented Oct 6, 2023

@SirYwell
The serviceability label was successfully added.

@mlbridge
Copy link

mlbridge bot commented Oct 6, 2023

Webrevs


void prepare() {
_next_klass = next_klass_with_fields();
// special case: the base klass has no fields. If any supertype has any fields, use that directly.

Choose a reason for hiding this comment

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

"base klass" sounds misleading.
I think "initial" would be clearer

@alexmenkov
Copy link

The fix itself looks good to me.
How did you tested the change?
Looks like we don't have test coverage for the correctness of the dumped fields. Would be nice to add it.

@SirYwell
Copy link
Member Author

The fix itself looks good to me. How did you tested the change? Looks like we don't have test coverage for the correctness of the dumped fields. Would be nice to add it.

Thanks.
I ran hotspot_serviceability and also manually looked into more complex heap dumps. I agree that specific tests would be better. I'll need to figure out how that can be accomplished. If you have any pointers how to get started there, please let me know.

@alexmenkov
Copy link

The fix itself looks good to me. How did you tested the change? Looks like we don't have test coverage for the correctness of the dumped fields. Would be nice to add it.

Thanks. I ran hotspot_serviceability and also manually looked into more complex heap dumps. I agree that specific tests would be better. I'll need to figure out how that can be accomplished. If you have any pointers how to get started there, please let me know.

We have test library to parse hprof files in test/lib/jdk/test/lib/hprof
You can look at test/hotspot/jtreg/serviceability/jvmti/vthread/HeapDump/VThreadInHeapDump.java as an example of a test which generates heap dump for target application and verifies it contains expected data.

@openjdk openjdk bot removed the rfr Pull request is ready for review label Oct 13, 2023
@openjdk openjdk bot added the rfr Pull request is ready for review label Oct 13, 2023
@SirYwell
Copy link
Member Author

We have test library to parse hprof files in test/lib/jdk/test/lib/hprof You can look at test/hotspot/jtreg/serviceability/jvmti/vthread/HeapDump/VThreadInHeapDump.java as an example of a test which generates heap dump for target application and verifies it contains expected data.

Thanks, I added a test case that ensures that the instance fields are all present. This is a very basic test, but it covers super types and also makes sure the order of supertypes is correct. If you want me to add something, please let me know.

@alexmenkov
Copy link

Thanks, I added a test case that ensures that the instance fields are all present. This is a very basic test, but it covers super types and also makes sure the order of supertypes is correct. If you want me to add something, please let me know.

Could you add testcases for corner cases:
no fields:
interface I1 {
}
class NoFields1 {
}
class NoFields2 extends NoFields1 implements I1 {
}

no parent fields:
class NoParentFields extends NoFields1 implements I1 {
int i1 = 1;
int i2 = 2;
}

only parent fields:
class Parent1 {
int i3 = 3;
}
class OnlyParentFields extends Parent1 {
}

Iterable<JavaHeapObject> objects = snapshot.getThings()::asIterator;
for (JavaHeapObject heapObj : objects) {
if (heapObj instanceof JavaObject javaObj) {
if (javaObj.getClazz().getName().endsWith("$B")) {

Choose a reason for hiding this comment

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

Suggested change
if (javaObj.getClazz().getName().endsWith("$B")) {
if (javaObj.getClazz().getName().equals(FieldsInInstanceTarg.B.class.getName())) {

asString = fields.subList(0, 2).toString();
Asserts.assertTrue(asString.contains("3"), "value for field A.a not found");
Asserts.assertTrue(asString.contains("Field"), "value for field A.s not found");
System.out.println(fields);

Choose a reason for hiding this comment

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

Suggested change
System.out.println(fields);
log(fields);

And I think it makes sense to print field values before asserts (so they appear in the log if some assertion throw an exception)

log("Snapshot resolved.");

Iterable<JavaHeapObject> objects = snapshot.getThings()::asIterator;
for (JavaHeapObject heapObj : objects) {

Choose a reason for hiding this comment

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

Instead if iteration over all dumped objects it would be faster to:
(JavaObject)snapshot.findClass(className).getInstances(false).nextElement();

@SirYwell
Copy link
Member Author

Thanks, I added a test case that ensures that the instance fields are all present. This is a very basic test, but it covers super types and also makes sure the order of supertypes is correct. If you want me to add something, please let me know.

Could you add testcases for corner cases: no fields: interface I1 { } class NoFields1 { } class NoFields2 extends NoFields1 implements I1 { }

no parent fields: class NoParentFields extends NoFields1 implements I1 { int i1 = 1; int i2 = 2; }

only parent fields: class Parent1 { int i3 = 3; } class OnlyParentFields extends Parent1 { }

Done. I also added a test case where a class class in the "middle" class has no fields.

}

interface I {
int i = -10;

Choose a reason for hiding this comment

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

wrong indent

@openjdk
Copy link

openjdk bot commented Oct 16, 2023

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

8317692: jcmd GC.heap_dump performance regression after JDK-8292818

Reviewed-by: amenkov, fparain

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

  • defc7e0: 8318454: TestLayoutPaths broken on Big Endian platforms after JDK-8317837
  • 3c70f2c: 8318418: hsdis build fails with system binutils on Ubuntu
  • 15acf4b: 8318324: Drop redundant default methods from FFM API
  • 1a09835: 8317358: G1: Make TestMaxNewSize use createTestJvm
  • 47bb1a1: 8318415: Adjust describing comment of os_getChildren after 8315026
  • 80bd22d: 8316144: Remove unused field jdk.internal.util.xml.impl.XMLStreamWriterImpl.Element._Depth
  • c0e154c: 8318089: Class space not marked as such with NMT when CDS is off
  • 24bc5bd: 8318104: macOS 10.13 check in TabButtonAccessibility.m can be removed
  • e25a49a: 8318471: ProblemList compiler/sharedstubs/SharedTrampolineTest.java
  • ce8ebeb: 8317979: Use TZ database style abbreviations in the CLDR locale provider
  • ... and 90 more: https://git.openjdk.org/jdk/compare/9728e21db1b35e487c562690de659aac386aa99d...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.

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 (@alexmenkov, @dholmes-ora, @fparain) 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 openjdk bot added the ready Pull request is ready to be integrated label Oct 16, 2023
@SirYwell
Copy link
Member Author

Fixed the wrong indent. Thank you for your review. Do I need another one or can we proceed?

@alexmenkov
Copy link

Fixed the wrong indent. Thank you for your review. Do I need another one or can we proceed?

Hotspot changes require 2 reviewers

@SirYwell
Copy link
Member Author

/reviewers 2

@openjdk
Copy link

openjdk bot commented Oct 18, 2023

@SirYwell
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Oct 18, 2023
Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me but I'd really like @fparain to review this. Thanks,

Copy link
Contributor

@fparain fparain left a comment

Choose a reason for hiding this comment

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

Changes look good to me.
Thank you for fixing this.

Fred

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Oct 19, 2023
@SirYwell
Copy link
Member Author

Thank you all for your reviews.
I think this is worth a backport to jdk21u, what do you think? And if yes, is it enough to run the backport command on the commit later and create a PR from it?

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 19, 2023
@openjdk
Copy link

openjdk bot commented Oct 19, 2023

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

@plummercj
Copy link
Contributor

I think this is worth a backport to jdk21u, what do you think? And if yes, is it enough to run the backport command on the commit later and create a PR from it?

Since the problem was introduced in 21, that sounds reasonable. You can use the backport command, but make sure you first follow the approval process described at https://openjdk.org/projects/jdk-updates/approval.html

@dholmes-ora
Copy link
Member

/sponsor

@openjdk
Copy link

openjdk bot commented Oct 19, 2023

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

  • 684b91e: 8315064: j.text.ChoiceFormat provides no specification on quoting behavior
  • 1740950: 8314901: AES-GCM interleaved implementation using AVX2 instructions
  • cc8f8da: 8318322: Update IANA Language Subtag Registry to Version 2023-10-16
  • 599560a: 8317635: Improve GetClassFields test to verify correctness of field order
  • 9cf334f: 8318383: Remove duplicated checks in os::get_native_stack() in posix implementation
  • defc7e0: 8318454: TestLayoutPaths broken on Big Endian platforms after JDK-8317837
  • 3c70f2c: 8318418: hsdis build fails with system binutils on Ubuntu
  • 15acf4b: 8318324: Drop redundant default methods from FFM API
  • 1a09835: 8317358: G1: Make TestMaxNewSize use createTestJvm
  • 47bb1a1: 8318415: Adjust describing comment of os_getChildren after 8315026
  • ... and 95 more: https://git.openjdk.org/jdk/compare/9728e21db1b35e487c562690de659aac386aa99d...master

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Oct 19, 2023

@dholmes-ora @SirYwell Pushed as commit 8f5f440.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated serviceability serviceability-dev@openjdk.org
5 participants