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

8005885: enhance PrintCodeCache to print more data #7389

Closed
wants to merge 10 commits into from

Conversation

yftsai
Copy link
Contributor

@yftsai yftsai commented Feb 8, 2022


Progress

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

Issue

  • JDK-8005885: enhance PrintCodeCache to print more data

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7389

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 8, 2022

👋 Welcome back yftsai! 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
Copy link

@openjdk openjdk bot commented Feb 8, 2022

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

  • hotspot-compiler

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-compiler label Feb 8, 2022
} else {
live.add(cb);
if (cb->is_nmethod()) {
const int level = cb->as_nmethod_or_null()->comp_level();
Copy link
Member

@navyxliu navyxliu Feb 8, 2022

Choose a reason for hiding this comment

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

is cb->as_nmethod() better?

live.print("live");
for (int i = CompLevel_simple; i <= CompLevel_full_optimization; i++) {
tty->print_cr("Tier %d:", i);
if (!live[i - 1].is_empty()) {
Copy link
Member

@navyxliu navyxliu Feb 8, 2022

Choose a reason for hiding this comment

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

your policy is to skip empty entries. I suggest you move is_empty() logic to CodeBlob_sizes::print().
one side, it would print out "#0 live: empty" instead of skipping it. that's informative.
secondly, it should simplify your code here.

@yftsai yftsai marked this pull request as ready for review Feb 16, 2022
@openjdk openjdk bot added the rfr label Feb 16, 2022
@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 16, 2022

Webrevs

Copy link
Member

@navyxliu navyxliu left a comment

My comments are nits. Generally, this PR looks good to me.
I am not a reviewer. We still need other reviewers to approve it.


void print(const char* title) const {
if (is_empty())
{
Copy link
Member

@navyxliu navyxliu Feb 17, 2022

Choose a reason for hiding this comment

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

better stick with the existing codestyle. they always use if (cond) { and else {

{ "other", &other },
};
tty->print_cr("Stubs:");
for (auto &stub: stubs) {
Copy link
Member

@navyxliu navyxliu Feb 17, 2022

Choose a reason for hiding this comment

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

current code style prefer auto& stub. same as 1486 1487 lines, asterisk associates with type.

Copy link
Member

@phohensee phohensee left a comment

Why not print both absolute size and percentage? E.g., "24K(10%)".

Could include sample output in a comment please?

struct {
const char* name;
const CodeBlob_sizes* sizes;
} stubs[] = {
Copy link
Contributor

@eastig eastig Feb 22, 2022

Choose a reason for hiding this comment

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

As stub has a particular meaning and there are non-stubs: adapters buffers and other, I suggest non_nmethod_blobs instead of stubs.

{ "buffer blob", &bufferBlob },
{ "other", &other },
};
tty->print_cr("Stubs:");
Copy link
Contributor

@eastig eastig Feb 22, 2022

Choose a reason for hiding this comment

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

Non-nmethod blobs:

for (int i = 0; i <= CompLevel_full_optimization; i++) {
tty->print_cr("Tier %d:", i);
Copy link
Contributor

@eastig eastig Feb 22, 2022

Choose a reason for hiding this comment

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

What about to do it more informative:

Nmethod blobs per compilation level:
CompLevel_none:
...
CompLevel_simple:
...

@yftsai
Copy link
Contributor Author

@yftsai yftsai commented Feb 22, 2022

A snippet from java -XX:+TieredCompilation -XX:+UnlockDiagnosticVMOptions -XX:+PrintCodeCache -XX:+Verbose

nmethod blobs per compilation level:
none:
 #45 live = 38K (hdr 16K 41%, loc 4K 10%, code 18K 46%, stub 0K 0%, [oops 0K 0%, metadata 0K 0%, data 0K 0%, pcs 0K 0%])
 #0 dead = 0K
simple:
 #56 live = 43K (hdr 20K 46%, loc 3K 6%, code 9K 22%, stub 4K 11%, [oops 0K 0%, metadata 0K 1%, data 0K 1%, pcs 3K 7%])
 #0 dead = 0K
limited profile:
 #0 live = 0K
 #0 dead = 0K
full profile:
 #310 live = 786K (hdr 111K 14%, loc 61K 7%, code 360K 45%, stub 38K 4%, [oops 0K 0%, metadata 7K 0%, data 68K 8%, pcs 124K 15%])
 #0 dead = 0K
full optimization:
 #38 live = 114K (hdr 13K 11%, loc 6K 5%, code 45K 39%, stub 1K 1%, [oops 0K 0%, metadata 1K 1%, data 14K 12%, pcs 28K 25%])
 #0 dead = 0K
Non-nmethod blobs:
 #60 runtime = 34K (hdr 7K 20%, loc 1K 5%, code 24K 72%, stub 0K 0%, [oops 0K 0%, metadata 0K 0%, data 0K 0%, pcs 0K 0%])
 #1 uncommon trap = 0K (hdr 0K 20%, loc 0K 4%, code 0K 76%, stub 0K 0%, [oops 0K 0%, metadata 0K 0%, data 0K 0%, pcs 0K 0%])
 #1 deoptimization = 1K (hdr 0K 8%, loc 0K 1%, code 1K 89%, stub 0K 0%, [oops 0K 0%, metadata 0K 0%, data 0K 0%, pcs 0K 0%])
 #667 adapter = 512K (hdr 78K 15%, loc 45K 8%, code 376K 73%, stub 0K 0%, [oops 0K 0%, metadata 0K 0%, data 0K 0%, pcs 0K 0%])
 #17 buffer blob = 3163K (hdr 1K 0%, loc 0K 0%, code 3161K 99%, stub 0K 0%, [oops 0K 0%, metadata 0K 0%, data 0K 0%, pcs 0K 0%])
 #4 other = 2K (hdr 0K 18%, loc 0K 2%, code 1K 77%, stub 0K 0%, [oops 0K 0%, metadata 0K 0%, data 0K 0%, pcs 0K 0%])

case CompLevel_none: level_name = "CompLevel_none"; break;
case CompLevel_simple: level_name = "CompLevel_simple"; break;
case CompLevel_limited_profile: level_name = "CompLevel_limited_profile"; break;
case CompLevel_full_profile: level_name = "CompLevel_full_profile"; break;
case CompLevel_full_optimization: level_name = "CompLevel_full_optimization"; break;
default: assert(false, "invalid compLevel");
Copy link
Contributor

@eastig eastig Feb 22, 2022

Choose a reason for hiding this comment

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

Could you please put 'level_name = ; break;' into separate lines? It should improve readability .

Copy link
Contributor Author

@yftsai yftsai Feb 22, 2022

Choose a reason for hiding this comment

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

They're better aligned now though no separate lines were added.

src/hotspot/share/code/codeCache.cpp Show resolved Hide resolved
for (int i = 0; i <= CompLevel_full_optimization; i++) {
const char *level_name;
switch (i) {
case CompLevel_none: level_name = "CompLevel_none"; break;
Copy link
Member

@phohensee phohensee Feb 22, 2022

Choose a reason for hiding this comment

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

It would be less verbose if you removed the "CompLevel_" prefix and the underscores in the name strings. I.e., "none", "simple", "limited profile", "full profile", "full optimization".

Copy link
Member

@phohensee phohensee left a comment

Lgtm.

@openjdk
Copy link

@openjdk openjdk bot commented Feb 22, 2022

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

8005885: enhance PrintCodeCache to print more data

Reviewed-by: xliu, phh

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

  • 23995f8: 8281525: Enable Zc:strictStrings flag in Visual Studio build
  • 20e78f7: 8282307: Parallel: Incorrect discovery mode in PCReferenceProcessor
  • 0b6862e: 8282348: Remove unused CardTable::dirty_card_iterate
  • 6fab8a2: 8277204: Implement PAC-RET branch protection on Linux/AArch64
  • abc0ce1: 8282316: Operation before String case conversion
  • 0796620: 8281944: JavaDoc throws java.lang.IllegalStateException: ERRONEOUS
  • 231e48f: 8282200: ShouldNotReachHere() reached by AsyncGetCallTrace after JDK-8280422
  • f4486a1: 8262400: runtime/exceptionMsgs/AbstractMethodError/AbstractMethodErrorTest.java fails in test_ame5_compiled_vtable_stub with wrapper
  • 3cfffa4: 8282188: Unused static field MathContext.DEFAULT_DIGITS
  • 379fd85: 8277369: Strange behavior of JMenuBar with RIGHT_TO_LEFT orientation, arrow keys behaves opposite traversing through keyboard
  • ... and 229 more: https://git.openjdk.java.net/jdk/compare/47800bf3da181ae0ee612b14d95773fd1dc90350...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 (@navyxliu, @phohensee) 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 label Feb 22, 2022
eastig
eastig approved these changes Feb 22, 2022
Copy link
Contributor

@eastig eastig left a comment

lgtm

@neliasso
Copy link

@neliasso neliasso commented Feb 22, 2022

Have you verified that this change doesn't break any tests?

@TobiHartmann
Copy link
Member

@TobiHartmann TobiHartmann commented Feb 23, 2022

Have you verified that this change doesn't break any tests?

Just wanted to mention that there are several tests that match the output of -XX:+PrintCodeCache, for example test/hotspot/jtreg/vmTestbase/vm/compiler/CodeCacheInfo/Test.java. Please verify that they still work.

@yftsai
Copy link
Contributor Author

@yftsai yftsai commented Feb 23, 2022

Have you verified that this change doesn't break any tests?

Just wanted to mention that there are several tests that match the output of -XX:+PrintCodeCache, for example test/hotspot/jtreg/vmTestbase/vm/compiler/CodeCacheInfo/Test.java. Please verify that they still work.

No regression was found in tier 2 and 3 tests, and the following tests all pass. There is no change in the output of -XX:+PrintCodeCache if -XX:+Verbose is not specified.

jtreg:test/hotspot/jtreg/compiler/codecache/CheckSegmentedCodeCache.java
jtreg:test/hotspot/jtreg/compiler/codecache/cli/printcodecache/TestPrintCodeCacheOption.java
jtreg:test/hotspot/jtreg/compiler/codecache/cli/codeheapsize/TestCodeHeapSizeOptions.java
jtreg:test/hotspot/jtreg/compiler/codecache/cli/TestSegmentedCodeCacheOption.java
jtreg:test/hotspot/jtreg/vmTestbase/vm/compiler/CodeCacheInfo/Test.java
jtreg:test/hotspot/jtreg/vmTestbase/vm/compiler/CodeCacheInfoOnCompilation/Test.java
jtreg:test/jdk/com/sun/management/HotSpotDiagnosticMXBean/CheckOrigin.java

@TobiHartmann
Copy link
Member

@TobiHartmann TobiHartmann commented Feb 23, 2022

Okay, thanks for verifying!

Copy link
Member

@phohensee phohensee left a comment

Latest lgtm.

Copy link
Contributor

@eastig eastig left a comment

Two minor comments.

@@ -0,0 +1,78 @@
/*
* Copyright (c) 2022, Amazon.com Inc. or its affiliates. All rights reserved.
Copy link
Contributor

@eastig eastig Feb 24, 2022

Choose a reason for hiding this comment

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

We don't specify a year any more.
The correct one is "Copyright Amazon.com Inc. or its affiliates. All Rights Reserved." See #6915

@yftsai
Copy link
Contributor Author

@yftsai yftsai commented Feb 24, 2022

/integrate

@openjdk openjdk bot added the sponsor label Feb 24, 2022
@openjdk
Copy link

@openjdk openjdk bot commented Feb 24, 2022

@yftsai
Your change (at version 89f1835) is now ready to be sponsored by a Committer.

Copy link
Member

@phohensee phohensee left a comment

One final time, lgtm.

@phohensee
Copy link
Member

@phohensee phohensee commented Feb 24, 2022

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Feb 24, 2022

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

  • 23995f8: 8281525: Enable Zc:strictStrings flag in Visual Studio build
  • 20e78f7: 8282307: Parallel: Incorrect discovery mode in PCReferenceProcessor
  • 0b6862e: 8282348: Remove unused CardTable::dirty_card_iterate
  • 6fab8a2: 8277204: Implement PAC-RET branch protection on Linux/AArch64
  • abc0ce1: 8282316: Operation before String case conversion
  • 0796620: 8281944: JavaDoc throws java.lang.IllegalStateException: ERRONEOUS
  • 231e48f: 8282200: ShouldNotReachHere() reached by AsyncGetCallTrace after JDK-8280422
  • f4486a1: 8262400: runtime/exceptionMsgs/AbstractMethodError/AbstractMethodErrorTest.java fails in test_ame5_compiled_vtable_stub with wrapper
  • 3cfffa4: 8282188: Unused static field MathContext.DEFAULT_DIGITS
  • 379fd85: 8277369: Strange behavior of JMenuBar with RIGHT_TO_LEFT orientation, arrow keys behaves opposite traversing through keyboard
  • ... and 229 more: https://git.openjdk.java.net/jdk/compare/47800bf3da181ae0ee612b14d95773fd1dc90350...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label Feb 24, 2022
@openjdk openjdk bot closed this Feb 24, 2022
@openjdk openjdk bot removed ready rfr sponsor labels Feb 24, 2022
@openjdk
Copy link

@openjdk openjdk bot commented Feb 24, 2022

@phohensee @yftsai Pushed as commit b6843a1.

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

@yftsai yftsai deleted the cclogs branch Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler integrated
6 participants