8313562: hsperfdata should export module path and "launcher" metadata #19287
Conversation
|
/integrate |
|
👋 Welcome back larry-cable! A progress list of the required criteria for merging this PR into |
|
@larry-cable 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 50 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 (@dholmes-ora, @kevinjwalls) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
@larry-cable This pull request has not yet been marked as ready for integration. |
|
@larry-cable The following label 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 list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
|
|
||
| add_property_constant(JAVA_PROPERTY, "jdk.module.path", CHECK); | ||
| add_property_constant(JAVA_PROPERTY, "jdk.module.upgrade.path", CHECK); | ||
| add_property_constant(JAVA_PROPERTY, "jdk.module.main", CHECK); |
There was a problem hiding this comment.
These are not "java" properties - do we need to add a "jdk" namespace?
There was a problem hiding this comment.
correct @dholmes-ora however a couple of points to note:
- these properties are listed in the javadoc for System::getProperties also "jdk.debug" is a pre-existing property also present in the JAVA_PROPERTY NS ... which somewhat sets an awkward precedent...
happy to add a new JDK_PROPERTY NS but given the pre-existence of "jdk.debug" not clear to me that such would not introduce an arbitrary inconsistency
thoughts?
There was a problem hiding this comment.
I'm not sure what the list of system properties in System.get_properties has to do with this. That list includes a bunch of properties in the java namespace and a bunch not.
The pre-existence of jdk.debug is unfortunate and, I think, a mistake. As you can see from the JBS issue description the original intent was to have java.vm.debug but it got changed to jdk.debug:
https://bugs.openjdk.org/browse/JDK-8139986
I think the question that needs to be answered is why do these different namespaces even exist? AFAICT the SUN_* namespace is used for internal/implementation things and arguably if we could have we might have renamed that to JDK_*. But the reality is that we are dealing with an archaic part of the system that really reflects an age long past and could do with some modernization.
So I guess for the sake of moving forward we can ignore the namespace for now and just flag these as "Java" properties.
|
happy to do so although since their counterparts are listed in
System::getProperties javadoc I assumed that the java namespace
was sufficient.
- Larry
…On 5/19/24 6:27 PM, David Holmes wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In src/hotspot/share/runtime/statSampler.cpp
<https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/19287*discussion_r1606153336__;Iw!!ACWV5N9M2RV99hQ!J2zDS-JPZ6lYQCvMtioTgnwy2xyeXv7suGsbXKrZ2nVivFlTC417bpvocgXEm8XKPz4g4ZmSVZ7LUZ9FMKpGKpTb0Q$>:
> +
+ add_property_constant(JAVA_PROPERTY, "jdk.module.path", CHECK);
+ add_property_constant(JAVA_PROPERTY, "jdk.module.upgrade.path", CHECK);
+ add_property_constant(JAVA_PROPERTY, "jdk.module.main", CHECK);
These are not "java" properties - do we need to add a "jdk" namespace?
—
Reply to this email directly, view it on GitHub
<https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/19287*pullrequestreview-2065206278__;Iw!!ACWV5N9M2RV99hQ!J2zDS-JPZ6lYQCvMtioTgnwy2xyeXv7suGsbXKrZ2nVivFlTC417bpvocgXEm8XKPz4g4ZmSVZ7LUZ9FMKoDdBf2XA$>,
or unsubscribe
<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ANTA67UX7E7U7AU4SIHC4NTZDFGR5AVCNFSM6AAAAABH4SJC22VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANRVGIYDMMRXHA__;!!ACWV5N9M2RV99hQ!J2zDS-JPZ6lYQCvMtioTgnwy2xyeXv7suGsbXKrZ2nVivFlTC417bpvocgXEm8XKPz4g4ZmSVZ7LUZ9FMKoAsCXxPg$>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--------------VXlZDC786aXcbpQh93JFAVU8
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit
<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
happy to do so although since their counterparts are listed in
System::getProperties javadoc I assumed that the java namespace<br>
was sufficient.<br>
<br>
- Larry<br>
<br>
<div class="moz-cite-prefix">On 5/19/24 6:27 PM, David Holmes wrote:<br>
</div>
<blockquote type="cite" ***@***.***">
***@***.***</b> commented on this pull request.</p>
<hr>
<p>In <a href="https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/19287*discussion_r1606153336__;Iw!!ACWV5N9M2RV99hQ!J2zDS-JPZ6lYQCvMtioTgnwy2xyeXv7suGsbXKrZ2nVivFlTC417bpvocgXEm8XKPz4g4ZmSVZ7LUZ9FMKpGKpTb0Q$" moz-do-not-send="true">src/hotspot/share/runtime/statSampler.cpp</a>:</p>
<pre style="color:#555">> +
+ add_property_constant(JAVA_PROPERTY, "jdk.module.path", CHECK);
+ add_property_constant(JAVA_PROPERTY, "jdk.module.upgrade.path", CHECK);
+ add_property_constant(JAVA_PROPERTY, "jdk.module.main", CHECK);
</pre>
<p dir="auto">These are not "java" properties - do we need to add
a "jdk" namespace?</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br>
Reply to this email directly, <a href="https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/19287*pullrequestreview-2065206278__;Iw!!ACWV5N9M2RV99hQ!J2zDS-JPZ6lYQCvMtioTgnwy2xyeXv7suGsbXKrZ2nVivFlTC417bpvocgXEm8XKPz4g4ZmSVZ7LUZ9FMKoDdBf2XA$" moz-do-not-send="true">view it on GitHub</a>, or <a href="https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ANTA67UX7E7U7AU4SIHC4NTZDFGR5AVCNFSM6AAAAABH4SJC22VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANRVGIYDMMRXHA__;!!ACWV5N9M2RV99hQ!J2zDS-JPZ6lYQCvMtioTgnwy2xyeXv7suGsbXKrZ2nVivFlTC417bpvocgXEm8XKPz4g4ZmSVZ7LUZ9FMKoAsCXxPg$" moz-do-not-send="true">unsubscribe</a>.<br>
You are receiving this because you were mentioned.<img src="https://github.com/notifications/beacon/ANTA67XWTXTQWDHXMA3FCILZDFGR5A5CNFSM6AAAAABH4SJC22WGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTT3DCGAM.gif" alt="" moz-do-not-send="true" width="1" height="1"><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message
ID: <span><openjdk/jdk/pull/19287/review/2065206278</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
***@***.***": "http://schema.org",
***@***.***": "EmailMessage",
"potentialAction": {
***@***.***": "ViewAction",
"target": "#19287 (review)",
"url": "#19287 (review)",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
***@***.***": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>
</blockquote>
<br>
</body>
</html>
--------------VXlZDC786aXcbpQh93JFAVU8--
|
|
I also note that "jdk.debug property is a JAVA_PROPERTY, so not sure
what to do here in the presence of this potential "precident"?
- Larry
…On 5/19/24 6:27 PM, David Holmes wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In src/hotspot/share/runtime/statSampler.cpp
<https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/19287*discussion_r1606153336__;Iw!!ACWV5N9M2RV99hQ!J2zDS-JPZ6lYQCvMtioTgnwy2xyeXv7suGsbXKrZ2nVivFlTC417bpvocgXEm8XKPz4g4ZmSVZ7LUZ9FMKpGKpTb0Q$>:
> +
+ add_property_constant(JAVA_PROPERTY, "jdk.module.path", CHECK);
+ add_property_constant(JAVA_PROPERTY, "jdk.module.upgrade.path", CHECK);
+ add_property_constant(JAVA_PROPERTY, "jdk.module.main", CHECK);
These are not "java" properties - do we need to add a "jdk" namespace?
—
Reply to this email directly, view it on GitHub
<https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/19287*pullrequestreview-2065206278__;Iw!!ACWV5N9M2RV99hQ!J2zDS-JPZ6lYQCvMtioTgnwy2xyeXv7suGsbXKrZ2nVivFlTC417bpvocgXEm8XKPz4g4ZmSVZ7LUZ9FMKoDdBf2XA$>,
or unsubscribe
<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ANTA67UX7E7U7AU4SIHC4NTZDFGR5AVCNFSM6AAAAABH4SJC22VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANRVGIYDMMRXHA__;!!ACWV5N9M2RV99hQ!J2zDS-JPZ6lYQCvMtioTgnwy2xyeXv7suGsbXKrZ2nVivFlTC417bpvocgXEm8XKPz4g4ZmSVZ7LUZ9FMKoAsCXxPg$>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--------------7k0FeE9fQIGLR7ZxNRvoacUT
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit
<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
I also note that "jdk.debug property is a JAVA_PROPERTY, so not sure
what to do here in the presence of this potential "precident"?<br>
<br>
- Larry<br>
<br>
<div class="moz-cite-prefix">On 5/19/24 6:27 PM, David Holmes wrote:<br>
</div>
<blockquote type="cite" ***@***.***">
***@***.***</b> commented on this pull request.</p>
<hr>
<p>In <a href="https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/19287*discussion_r1606153336__;Iw!!ACWV5N9M2RV99hQ!J2zDS-JPZ6lYQCvMtioTgnwy2xyeXv7suGsbXKrZ2nVivFlTC417bpvocgXEm8XKPz4g4ZmSVZ7LUZ9FMKpGKpTb0Q$" moz-do-not-send="true">src/hotspot/share/runtime/statSampler.cpp</a>:</p>
<pre style="color:#555">> +
+ add_property_constant(JAVA_PROPERTY, "jdk.module.path", CHECK);
+ add_property_constant(JAVA_PROPERTY, "jdk.module.upgrade.path", CHECK);
+ add_property_constant(JAVA_PROPERTY, "jdk.module.main", CHECK);
</pre>
<p dir="auto">These are not "java" properties - do we need to add
a "jdk" namespace?</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br>
Reply to this email directly, <a href="https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/19287*pullrequestreview-2065206278__;Iw!!ACWV5N9M2RV99hQ!J2zDS-JPZ6lYQCvMtioTgnwy2xyeXv7suGsbXKrZ2nVivFlTC417bpvocgXEm8XKPz4g4ZmSVZ7LUZ9FMKoDdBf2XA$" moz-do-not-send="true">view it on GitHub</a>, or <a href="https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ANTA67UX7E7U7AU4SIHC4NTZDFGR5AVCNFSM6AAAAABH4SJC22VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANRVGIYDMMRXHA__;!!ACWV5N9M2RV99hQ!J2zDS-JPZ6lYQCvMtioTgnwy2xyeXv7suGsbXKrZ2nVivFlTC417bpvocgXEm8XKPz4g4ZmSVZ7LUZ9FMKoAsCXxPg$" moz-do-not-send="true">unsubscribe</a>.<br>
You are receiving this because you were mentioned.<img src="https://github.com/notifications/beacon/ANTA67XWTXTQWDHXMA3FCILZDFGR5A5CNFSM6AAAAABH4SJC22WGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTT3DCGAM.gif" alt="" moz-do-not-send="true" width="1" height="1"><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message
ID: <span><openjdk/jdk/pull/19287/review/2065206278</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
***@***.***": "http://schema.org",
***@***.***": "EmailMessage",
"potentialAction": {
***@***.***": "ViewAction",
"target": "#19287 (review)",
"url": "#19287 (review)",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
***@***.***": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>
</blockquote>
<br>
</body>
</html>
--------------7k0FeE9fQIGLR7ZxNRvoacUT--
|
|
/integrate |
|
@larry-cable |
…d module properties
There was a problem hiding this comment.
The update is good, yes those can be nulls 8-)
Do you see a compiler warning for the if being all on one line, in line 239 of statSampler.cpp? I do, locally, and I need to use if () { } to avoid a build failure.
Then running with: --module-path=/foo
I see:
jcmd 14540 PerfCounter.print | grep -i modu
java.property.jdk.module.path="/foo"
java.rt.vmArgs="--module-path=/foo"
jdk.module.finder.modulepath.modules=0
jdk.module.finder.modulepath.scanTime=10411013
JDK-8313562 doesn't specify what information will be included and what perfdata names will be, but I think this is what was intended.
Maybe a (C) update while you're there, and it's done.
hsperfdata should expose module metadata if available.
Progress
Warning
8313562: hsperfdata should export module path and "launcher" metadataIssue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19287/head:pull/19287$ git checkout pull/19287Update a local copy of the PR:
$ git checkout pull/19287$ git pull https://git.openjdk.org/jdk.git pull/19287/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19287View PR using the GUI difftool:
$ git pr show -t 19287Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19287.diff
Webrev
Link to Webrev Comment