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
