Hi Neil,
I tried out this patch with my hotspot, it does not work.
The reason is that, the environment variable is setup too late, it has
to be set before it launches JavaVM (before calling LoadJavaVM()) method.
Thanks,
-Zhengyu
On 6/25/2014 1:58 PM, Neil Toda wrote:
Sorry. One more webrev .. 06
http://cr.openjdk.java.net/~ntoda/8042469/webrev-06/
Kumar's nit was correct and in fact index was old and should have been
removed
allowing .contains member to be used instead of .indexOf. So cleaned
up a bit more
as can be seen below. Other of Kumar's nits done.
Thanks Kumar.
webrev-06
102 // get the PID from the env var we set for the JVM
103 String envVarPid = null;
104 for (String line : tr.testOutput) {
105 if (line.contains(envVarPidString)) {
106 int sindex = envVarPidString.length();
107 envVarPid = line.substring(sindex);
108 break;
109 }
110 }
111 // did we find envVarPid?
112 if (envVarPid == null) {
113 System.out.println(tr);
114 throw new RuntimeException("Error: failed to find env Var Pid
in tracking info");
115 }
webrev-05
102 // get the PID from the env var we set for the JVM
103 haveLauncherPid = false;
104 String envVarPid = null;
105 for (String line : tr.testOutput) {
106 int index;
107 if ((index = line.indexOf(envVarPidString)) >= 0) {
108 int sindex = envVarPidString.length();
109 envVarPid = line.substring(sindex);
110 haveLauncherPid = true;
111 break;
112 }
113 }
114 // did we find envVarPid?
115 if (!haveLauncherPid) {
116 System.out.println(tr);
117 throw new RuntimeException("Error: failed to find env Var Pid
in tracking info");
118 }
On 6/24/2014 2:28 PM, Neil Toda wrote:
New webrev-05 with Kumar's comments except for the "C'ish" style.
Explained below.
http://cr.openjdk.java.net/~ntoda/8042469/webrev-05/
105 : DONE
146: DONE
168: DONE
106: Your suggestion was the way I had originally coded it for
Linux. However
on Windows/Cygwin, the code will not compile There is some
interaction
with the doExec() preceeding it and the fact that it is a
varlist. This coding
was the simplest workaround.
Thanks for the nits Kumar.
On 6/24/2014 5:36 AM, Kumar Srinivasan wrote:
Neil,
Some nits:
TestSpecialArgs.java:
extra space
105 for ( String line : tr.testOutput) {
This is very C'ish, I suggest.
-106 int index;
-107 if ((index = line.indexOf(envVarPidString)) >=
0) {
+106 int index = line.indexOf(envVarPidString);
+107 if (index >= 0) {
Needs space
-146 launcherPid = line.substring(sindex,tindex);
+146 launcherPid = line.substring(sindex, tindex);
Needs space
-168 if (!tr.contains("NativeMemoryTracking: got value
"+NMT_Option_Value)) {
+168 if (!tr.contains("NativeMemoryTracking: got value "
+ NMT_Option_Value)) {
Thanks
Kumar
On 6/23/2014 10:26 AM, Neil Toda wrote:
I've spun a new webrev based on the comments for webrev-03:
http://cr.openjdk.java.net/~ntoda/8042469/webrev-04/
Changes are:
1) java.c
a) add free(envName) line 1063
b) change from malloc() to JLI_MemAlloc() @ lines 1048 and
1056
Thanks
-neil
On 6/20/2014 4:45 PM, Kumar Srinivasan wrote:
Neil,
Generally looks good, yes JLI_* functions must be used, I missed
that one.
Are you going to post another iteration ?
Kumar
On 6/20/2014 4:27 PM, Neil Toda wrote:
Thanks Joe. It would have checked for NULL for me.
I'll use the JLI wrapper.
-neil
On 6/20/2014 4:04 PM, Joe Darcy wrote:
Memory allocation in the launcher should use one of the
JLI_MemAlloc wrappers, if possible.
-Joe
On 06/20/2014 09:50 AM, Neil Toda wrote:
They should complain. Thanks Zhengyu. I'll make sure these
are non-null.
-neil
On 6/20/2014 5:01 AM, Zhengyu Gu wrote:
Neil,
Thanks for quick implementation.
java.c:
Did not check return values of malloc(), I wonder if source
code analyzers will complain.
-Zhengyu
On 6/19/2014 8:29 PM, Neil Toda wrote:
Launcher support for modified Native Memory Tracking
mechanism in JVM in JDK9.
Webrev : http://cr.openjdk.java.net/~ntoda/8042469/webrev-03/
bug : https://bugs.openjdk.java.net/browse/JDK-8042469
CCC : http://ccc.us.oracle.com/8042469
Thanks.
-neil
