RE: Proposal:JdpController.getProcessId() VM compatibility improvement
Hi Andrew,
I can help you with this contribution.
First question: Do you want to create a fix completely resolving Roger's bug
8074569 [1] or shall I create a new bug for you? In the latter case, please
send me a title and a long text (you can send this in a private Email).
Then, when we have the bug, you'll need to create a WebRev and post it for
review. Are you familiar with that process? Do you have some close colleague
who can assist you with that and upload a WebRev to cr.openjdk.java.net? If not
I can do that for you when you send me the data. We can discuss that in a
private mail thread as well.
Best regards
Christoph
[1] https://bugs.openjdk.java.net/browse/JDK-8074569
From: serviceability-dev [mailto:serviceability-dev-boun...@openjdk.java.net]
On Behalf Of Andrew Leonard
Sent: Donnerstag, 22. Juni 2017 18:31
To: serviceability-dev@openjdk.java.net
Subject: Fw: Proposal:JdpController.getProcessId() VM compatibility improvement
Hello,
I would like to propose the change below to the JdpController.getProcessId()
method which currently uses a reflection hack and the VMManagement interface.
Instead for JDK9+ we can use the ProcessHandle.current().getPid() method, which
will also allow better compatibility with other pluggable VMs.
jdk/src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java
36d35
< import sun.management.VMManagement;
136,149c135,136
< try {
< // Get the current process id using a reflection hack
< RuntimeMXBean runtime = ManagementFactory.getRuntimeMXBean();
< Field jvm = runtime.getClass().getDeclaredField("jvm");
< jvm.setAccessible(true);
<
< VMManagement mgmt = (sun.management.VMManagement)
jvm.get(runtime);
< Method pid_method =
mgmt.getClass().getDeclaredMethod("getProcessId");
< pid_method.setAccessible(true);
< Integer pid = (Integer) pid_method.invoke(mgmt);
< return pid;
< } catch(Exception ex) {
< return null;
< }
---
> // Get the current process id
> return (int)ProcessHandle.current().getPid();
I'd appreciate any feedback please, and help obtaining a suitable sponsor and
contributor?
Thanks
Andrew
Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
Re: Proposal:JdpController.getProcessId() VM compatibility improvement
Thanks Roger for taking a look, i'll see what the serviceability community
think,
cheers
Andrew
Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com
From: Roger Riggs <roger.ri...@oracle.com>
To: Andrew Leonard <andrew_m_leon...@uk.ibm.com>
Cc: serviceability-dev@openjdk.java.net, Core-Libs-Dev
<core-libs-...@openjdk.java.net>
Date: 26/06/2017 19:59
Subject:Re: Proposal:JdpController.getProcessId() VM compatibility
improvement
Hi Andrew,
Redirecting back to serviceability-dev@openjdk.java.net, it is the better
list for this topic.
I don't know these tests but it seems odd to stick that assert into every
callback to packetFromThisVMReceived. Perhaps someone more familiar can
review and sponsor.
Thanks, Roger
On 6/26/2017 10:37 AM, Andrew Leonard wrote:
Hi Roger,
I have now updated to the latest openjdk9 and examined the existing jdp
tests. I have added to those tests to assert the JDP packet processId,
which was in fact "null", so it was failing to set it before... Hence,
with this testcase update they fail without my patch, and succeed with the
new patch.
Being new to this contribution process, where do I go from here please?
Thanks
Andrew
diff -r 2425838cfb5e
src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java
---
a/src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java
Fri Jun 23 14:32:59 2017 -0400
+++
b/src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java
Mon Jun 26 15:31:15 2017 +0100
@@ -133,20 +133,8 @@
// Get the process id of the current running Java process
private static Integer getProcessId() {
-try {
-// Get the current process id using a reflection hack
-RuntimeMXBean runtime = ManagementFactory.getRuntimeMXBean();
-Field jvm = runtime.getClass().getDeclaredField("jvm");
-jvm.setAccessible(true);
-
-VMManagement mgmt = (sun.management.VMManagement)
jvm.get(runtime);
-Method pid_method =
mgmt.getClass().getDeclaredMethod("getProcessId");
-pid_method.setAccessible(true);
-Integer pid = (Integer) pid_method.invoke(mgmt);
-return pid;
-} catch(Exception ex) {
-return null;
-}
+// Get the current process id
+return (int)ProcessHandle.current().pid();
}
diff -r 2425838cfb5e test/sun/management/jdp/JdpOnTestCase.java
--- a/test/sun/management/jdp/JdpOnTestCase.javaFri Jun 23
14:32:59 2017 -0400
+++ b/test/sun/management/jdp/JdpOnTestCase.javaMon Jun 26
15:30:51 2017 +0100
@@ -31,6 +31,7 @@
import java.net.SocketTimeoutException;
import java.util.Map;
+import static jdk.testlibrary.Asserts.assertNotEquals;
public class JdpOnTestCase extends JdpTestCase {
@@ -58,6 +59,7 @@
final String jdpName = payload.get("INSTANCE_NAME");
log.fine("Received correct JDP packet #" +
String.valueOf(receivedJDPpackets) +
", jdp.name=" + jdpName);
+assertNotEquals(null, payload.get("PROCESS_ID"), "Expected
payload.get(\"PROCESS_ID\") to be not null.");
}
Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com
From:Roger Riggs <roger.ri...@oracle.com>
To:Andrew Leonard <andrew_m_leon...@uk.ibm.com>
Cc: core-libs-...@openjdk.java.net
Date:23/06/2017 13:57
Subject:Re: Proposal:JdpController.getProcessId() VM compatibility
improvement
Hi Leonard,
No need to stray from your intended focus.
The other is just another cleanup.
Thanks, Roger
On 6/23/17 6:57 AM, Andrew Leonard wrote:
Thanks Roger,
So yes that issue does look similar, although I was only looking in the
management interfaces. I'll have a peek at those references to see if they
are similar, it would make sense to clean those up too, to be consistent.
Your thoughts?
I'll also see if there's a junit for this method, if not i'll see if I can
add one.
Cheers
Andrew
Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com
From:Roger Riggs <roger.ri...@oracle.com>
To: core-libs-...@openjdk.java.net
Date:22/06/2017 18:03
Subject:Re: Proposal:JdpController.getProcessId() VM compatibility
improvement
Sent by:"core-libs-dev" <core-libs-dev-boun...@openjdk.java.net>
HI,
This looks like:
JDK-8074569 <https://bugs.openjdk.java.net/browse/JDK-8
Re: Proposal:JdpController.getProcessId() VM compatibility improvement
Hi Andrew,
Redirecting back to serviceability-dev@openjdk.java.net, it is the
better list for this topic.
I don't know these tests but it seems odd to stick that assert into every
callback to packetFromThisVMReceived. Perhaps someone more familiar can
review and sponsor.
Thanks, Roger
On 6/26/2017 10:37 AM, Andrew Leonard wrote:
Hi Roger,
I have now updated to the latest openjdk9 and examined the existing
jdp tests. I have added to those tests to assert the JDP packet
processId, which was in fact "null", so it was failing to set it
before... Hence, with this testcase update they fail without my patch,
and succeed with the new patch.
Being new to this contribution process, where do I go from here please?
Thanks
Andrew
diff -r 2425838cfb5e
src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java
---
a/src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java
Fri Jun 23 14:32:59 2017 -0400
+++
b/src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java
Mon Jun 26 15:31:15 2017 +0100
@@ -133,20 +133,8 @@
// Get the process id of the current running Java process
private static Integer getProcessId() {
-try {
- // Get the current process id using a reflection hack
- RuntimeMXBean runtime = ManagementFactory.getRuntimeMXBean();
- Field jvm = runtime.getClass().getDeclaredField("jvm");
- jvm.setAccessible(true);
-
- VMManagement mgmt = (sun.management.VMManagement) jvm.get(runtime);
- Method pid_method = mgmt.getClass().getDeclaredMethod("getProcessId");
- pid_method.setAccessible(true);
- Integer pid = (Integer) pid_method.invoke(mgmt);
- return pid;
-} catch(Exception ex) {
- return null;
-}
+// Get the current process id
+return (int)ProcessHandle.current().pid();
}
diff -r 2425838cfb5e test/sun/management/jdp/JdpOnTestCase.java
--- a/test/sun/management/jdp/JdpOnTestCase.javaFri Jun 23
14:32:59 2017 -0400
+++ b/test/sun/management/jdp/JdpOnTestCase.javaMon Jun 26
15:30:51 2017 +0100
@@ -31,6 +31,7 @@
import java.net.SocketTimeoutException;
import java.util.Map;
+import static jdk.testlibrary.Asserts.assertNotEquals;
public class JdpOnTestCase extends JdpTestCase {
@@ -58,6 +59,7 @@
final String jdpName = payload.get("INSTANCE_NAME");
log.fine("Received correct JDP packet #" +
String.valueOf(receivedJDPpackets) +
", jdp.name=" + jdpName);
+assertNotEquals(null, payload.get("PROCESS_ID"), "Expected
payload.get(\"PROCESS_ID\") to be not null.");
}
Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com
From: Roger Riggs <roger.ri...@oracle.com>
To: Andrew Leonard <andrew_m_leon...@uk.ibm.com>
Cc: core-libs-...@openjdk.java.net
Date: 23/06/2017 13:57
Subject: Re: Proposal:JdpController.getProcessId() VM compatibility
improvement
Hi Leonard,
No need to stray from your intended focus.
The other is just another cleanup.
Thanks, Roger
On 6/23/17 6:57 AM, Andrew Leonard wrote:
Thanks Roger,
So yes that issue does look similar, although I was only looking in
the management interfaces. I'll have a peek at those references to see
if they are similar, it would make sense to clean those up too, to be
consistent. Your thoughts?
I'll also see if there's a junit for this method, if not i'll see if I
can add one.
Cheers
Andrew
Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: _andrew_m_leon...@uk.ibm.com_
<mailto:andrew_m_leon...@uk.ibm.com>
From: Roger Riggs _<roger.ri...@oracle.com>_
<mailto:roger.ri...@oracle.com>
To: _core-libs-...@openjdk.java.net_
<mailto:core-libs-...@openjdk.java.net>
Date: 22/06/2017 18:03
Subject: Re: Proposal:JdpController.getProcessId() VM compatibility
improvement
Sent by: "core-libs-dev" _<core-libs-dev-boun...@openjdk.java.net>_
<mailto:core-libs-dev-boun...@openjdk.java.net>
HI,
This looks like:
JDK-8074569 <_https://bugs.openjdk.java.net/browse/JDK-8074569_> Update
implementation to use ProcessHandle.current() to get the PID
I'm happy to sponsor.
Roger
On 6/22/2017 12:27 PM, Andrew Leonard wrote:
> Thanks Alan,
> Yes, you're right the exception swallowing can be removed too.
> I'll re-post to serviceability-dev...
> Cheers
> Andrew
>
> Andrew Leonard
> Java Runtimes Development
> IBM Hursley
> IBM United Kingdom Ltd
> Phone internal: 245913, external: 01962 815913
> internet email: _andrew_m_le
