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
8275729: Qualified method names in CodeHeap Analytics #6200
Conversation
👋 Welcome back eastig! A progress list of the required criteria for merging this PR into |
@eastig The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good now. Old output can not tell us which class the method belongs to.
Old:
0x00007f6e91063010 (+0x00000010) 0x000000a0( 0K) none 0 480 nMethod (deopt) nmethod
0x00007f6e91063310 (+0x00000310) 0x000000f8( 0K) none 0 480 nMethod (active) name()Ljava/lang/String;
0x00007f6e91063610 (+0x00000610) 0x000000f8( 0K) none 0 480 nMethod (active) descriptor()Ljava/lang/module/ModuleDescriptor;
0x00007f6e91063910 (+0x00000910) 0x00000000( 0K) none 0 480 nMethod (active) getReferenceVolatile(Ljava/lang/Object;J)Ljava/lang/Object;
0x00007f6e91063d90 (+0x00000d90) 0x00000000( 0K) none 0 480 nMethod (active) hashCode()I
0x00007f6e91064190 (+0x00001190) 0x000000f8( 0K) c1 1 480 nMethod (active) name()Ljava/lang/String;
0x00007f6e91064490 (+0x00001490) 0x000000f8( 0K) c1 1 480 nMethod (active) modifiers()Ljava/util/Set;
0x00007f6e91064790 (+0x00001790) 0x000000f8( 0K) c1 1 480 nMethod (active) targets()Ljava/util/Set;
0x00007f6e91064a90 (+0x00001a90) 0x000000f8( 0K) c1 1 480 nMethod (active) source()Ljava/lang/String;
0x00007f6e91064d90 (+0x00001d90) 0x000000f8( 0K) c1 1 480 nMethod (active) isEmpty()Z
New:
0x00007f08adc94010 (+0x00000010) 0x00000150( 0K) c1 3 480 nMethod (deopt) nmethod
0x00007f08adc94390 (+0x00000390) 0x000001b0( 0K) c1 3 480 nMethod (active) java.lang.String.isLatin1()Z
0x00007f08adc94710 (+0x00000710) 0x00000258( 0K) c1 3 480 nMethod (active) jdk.internal.util.Preconditions.checkIndex(IILjava/util/function/BiFunction;)I
0x00007f08adc94b90 (+0x00000b90) 0x000004e8( 1K) c1 3 480 nMethod (deopt) nmethod
0x00007f08adc95310 (+0x00001310) 0x00000298( 0K) c1 3 480 nMethod (active) java.lang.StringLatin1.charAt([BI)C
0x00007f08adc95790 (+0x00001790) 0x000001a0( 0K) c1 3 480 nMethod (active) java.lang.String.checkIndex(II)V
0x00007f08adc95b10 (+0x00001b10) 0x00000170( 0K) c1 3 480 nMethod (active) java.lang.String.coder()B
0x00007f08adc95e90 (+0x00001e90) 0x000003e8( 0K) c1 3 480 nMethod (active) java.lang.String.hashCode()I
0x00007f08adc96490 (+0x00002490) 0x00000130( 0K) c1 3 480 nMethod (deopt) nmethod
0x00007f08adc96790 (+0x00002790) 0x00000210( 0K) c1 3 480 nMethod (active) java.lang.String.length()I
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
@eastig 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:
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 101 new commits pushed to the
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 (@kelthuzadx, @TobiHartmann) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
/integrate |
/sponsor |
Going to push as commit 8fc16f1.
Your commit was automatically rebased without conflicts. |
@phohensee @eastig Pushed as commit 8fc16f1. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
@@ -2334,6 +2335,11 @@ void CodeHeapState::print_names(outputStream* out, CodeHeap* heap) { | |||
Symbol* methSig = method->signature(); | |||
const char* methSigS = (methSig == NULL) ? NULL : methSig->as_C_string(); | |||
methSigS = (methSigS == NULL) ? "<method signature unavailable>" : methSigS; | |||
|
|||
Klass* klass = method->method_holder(); | |||
assert(klass->is_loader_alive(), "must be alive"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure klass
is always valid here and that its class loader has to be alive (i.e. the corresponding class hasn't been unloaded in the meantime)?
In https://bugs.openjdk.java.net/browse/JDK-8275729 you say that the Top50 list already has qualified names but as far as I know, that information is already collected in the aggregation step where it is safe. You now query this information in the reporting step.
I know we had problems due to access to dead methods before (see JDK-8219586: CodeHeap State Analytics processes dead nmethods and I just want to make sure we don't re-introduce such problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simonis
The code is guarded by checks:
// access nmethod and Method fields only if we own the CodeCache_lock.
// This fact is implicitly transported via nm != NULL.
if (nmethod_access_is_safe(nm)) {
...
bool get_name = (cbType == nMethod_inuse) || (cbType == nMethod_notused);
...
if (get_name) {
I was thinking whether I should use if (klass->is_loader_alive())
or assert(klass->is_loader_alive())
. I chose the assert because if it is safe to access Method
than its holder Klass
must be alive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
the code is safe. Not because of the checks cited by @eastig but because print_names() is only called if the required locks (Compile_lock and CodeCache_lock) have been continuously held since the aggregation step. See src/hotspot/share/compiler/compileBroker.cpp. A lot of effort has been spent to be less restrictive on print_names(), with no success.
Thanks for the enhancement.
Volker, I sponsored this before you posted your review. Evgeny, if it's a problem, please file a bug. |
No problem, I know I was late :) |
Thanks for reviewing @kelthuzadx and @TobiHartmann. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me, the change looks good.
@@ -2334,6 +2335,11 @@ void CodeHeapState::print_names(outputStream* out, CodeHeap* heap) { | |||
Symbol* methSig = method->signature(); | |||
const char* methSigS = (methSig == NULL) ? NULL : methSig->as_C_string(); | |||
methSigS = (methSigS == NULL) ? "<method signature unavailable>" : methSigS; | |||
|
|||
Klass* klass = method->method_holder(); | |||
assert(klass->is_loader_alive(), "must be alive"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
the code is safe. Not because of the checks cited by @eastig but because print_names() is only called if the required locks (Compile_lock and CodeCache_lock) have been continuously held since the aggregation step. See src/hotspot/share/compiler/compileBroker.cpp. A lot of effort has been spent to be less restrictive on print_names(), with no success.
Thanks for the enhancement.
The change caused failure in our testing: https://bugs.openjdk.java.net/browse/JDK-8276429 |
I don't think we need this assert just to print klass's name. |
Agree. I'll submit PR with the code:
|
Yes, I am currently testing similar fix:
Note, failed test is |
Is NULL method holder an acceptable situation? Could it be a sign of a bug? |
You are right, all methods should have class holders. I just followed code pattern.
I see, you want to have the same string instead of I will test your changes too. File PR and I will review and post testing results. |
BTW, you need to update Copyright year in file. |
This PR changes nmethods names in
METHOD NAMES for CodeHeap
section to be qualified.Testing:
make test TEST="gtest"
: Passedmake run-test TEST="tier1"
: Passedmake run-test TEST="tier2"
: Passedmake run-test TEST=serviceability/dcmd/compiler/CodeHeapAnalyticsMethodNames.java
: PassedProgress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6200/head:pull/6200
$ git checkout pull/6200
Update a local copy of the PR:
$ git checkout pull/6200
$ git pull https://git.openjdk.java.net/jdk pull/6200/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 6200
View PR using the GUI difftool:
$ git pr show -t 6200
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6200.diff