Ready for Review : 8042469 : Launcher changes for native memory tracking scalability enhancement
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
Hi Neil, 1054 envName = malloc(pbuflen); envName is never freed looks like. Probably not a big deal since this runs only if launcher is traced, but thought I'd mention it. Sent from my phone On Jun 19, 2014 8:30 PM, "Neil Toda" <neil.toda@oracle.com> 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
Thanks Vitaly. Best to cleanup. I'll make that change. -neil On 6/19/2014 5:39 PM, Vitaly Davidovich wrote:
Hi Neil,
1054 envName = malloc(pbuflen);
envName is never freed looks like. Probably not a big deal since this runs only if launcher is traced, but thought I'd mention it.
Sent from my phone
On Jun 19, 2014 8:30 PM, "Neil Toda" <neil.toda@oracle.com <mailto:neil.toda@oracle.com>> wrote:
Launcher support for modified Native Memory Tracking mechanism in JVM in JDK9.
Webrev : http://cr.openjdk.java.net/~ntoda/8042469/webrev-03/ <http://cr.openjdk.java.net/%7Entoda/8042469/webrev-03/> bug : https://bugs.openjdk.java.net/browse/JDK-8042469 CCC : http://ccc.us.oracle.com/8042469
Thanks.
-neil
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
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
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
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
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
Thanks Kumar for your other finds. I'll spin another webrev this evening and post it with the fixes to address everyone's points. 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
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
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 >
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 >> >
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 >>> >> >
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 >>>> >>> >> >
Okay, Very glad you checked. It really does need to go as early as your prototype suggested. I'll get right on it. -neil On 6/25/2014 12:05 PM, Zhengyu Gu wrote:
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 >>>>> >>>> >>> >> >
Hi Neil, There is still an issue. Apparently, you can NOT free the buffer for the environment variable. 678 char * pbuf = (char*)JLI_MemAlloc(pbuflen); 679 JLI_Snprintf(pbuf, pbuflen, "%s%d=%s", NMT_Env_Name, JLI_GetPid(), value); 680 retval = JLI_PutEnv(pbuf); 681 if (JLI_IsTraceLauncher()) { 682 char* envName; 683 char* envBuf; 684 685 // ensures that malloc successful 686 envName = (char*)JLI_MemAlloc(pbuflen); 687 JLI_Snprintf(envName, pbuflen, "%s%d", NMT_Env_Name, JLI_GetPid()); 688 689 printf("TRACER_MARKER: NativeMemoryTracking: env var is %s\n",envName); 690 printf("TRACER_MARKER: NativeMemoryTracking: putenv arg %s\n",pbuf); 691 envBuf = getenv(envName); 692 printf("TRACER_MARKER: NativeMemoryTracking: got value %s\n",envBuf); 693 free(envName); 694 } 695 696 free(pbuf); <<<<=== can not free this buffer 697 } You can experiment it move #696 to #681, you will see the test to fail. Linux putenv document says: *putenv*is very widely available, but it might or might not copy its argument, risking memory leaks. Thanks, -Zhengyu On 6/25/2014 4:40 PM, Neil Toda wrote:
Okay, Very glad you checked. It really does need to go as early as your prototype suggested. I'll get right on it.
-neil
On 6/25/2014 12:05 PM, Zhengyu Gu wrote:
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 >>>>>> >>>>> >>>> >>> >> >
You are right. I found this upon looking: https://www.securecoding.cert.org/confluence/display/seccode/POS34-C.+Do+not... We moved to malloc since the launcher doesn't know about the value being passed with the native memory flag. We can put some limits on value, but that means the launcher making a few more decisions. We'll think on this a bit. What are the possible values for -XX:NativeMemoryTracking For the future, are there any other possibilities? Thanks -neil On 6/26/2014 10:57 AM, Zhengyu Gu wrote:
Hi Neil,
There is still an issue. Apparently, you can NOT free the buffer for the environment variable.
678 char * pbuf = (char*)JLI_MemAlloc(pbuflen); 679 JLI_Snprintf(pbuf, pbuflen, "%s%d=%s", NMT_Env_Name, JLI_GetPid(), value); 680 retval = JLI_PutEnv(pbuf); 681 if (JLI_IsTraceLauncher()) { 682 char* envName; 683 char* envBuf; 684 685 // ensures that malloc successful 686 envName = (char*)JLI_MemAlloc(pbuflen); 687 JLI_Snprintf(envName, pbuflen, "%s%d", NMT_Env_Name, JLI_GetPid()); 688 689 printf("TRACER_MARKER: NativeMemoryTracking: env var is %s\n",envName); 690 printf("TRACER_MARKER: NativeMemoryTracking: putenv arg %s\n",pbuf); 691 envBuf = getenv(envName); 692 printf("TRACER_MARKER: NativeMemoryTracking: got value %s\n",envBuf); 693 free(envName); 694 } 695 696 free(pbuf); <<<<=== can not free this buffer 697 }
You can experiment it move #696 to #681, you will see the test to fail.
Linux putenv document says:
*putenv*is very widely available, but it might or might not copy its argument, risking memory leaks.
Thanks,
-Zhengyu
On 6/25/2014 4:40 PM, Neil Toda wrote:
Okay, Very glad you checked. It really does need to go as early as your prototype suggested. I'll get right on it.
-neil
On 6/25/2014 12:05 PM, Zhengyu Gu wrote:
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 >>>>>>> >>>>>> >>>>> >>>> >>> >> >
The possible values are: off, summary and detail Thanks, -Zhengyu On 6/26/2014 3:58 PM, Neil Toda wrote:
You are right. I found this upon looking:
https://www.securecoding.cert.org/confluence/display/seccode/POS34-C.+Do+not...
We moved to malloc since the launcher doesn't know about the value being passed with the native memory flag. We can put some limits on value, but that means the launcher making a few more decisions. We'll think on this a bit.
What are the possible values for -XX:NativeMemoryTracking For the future, are there any other possibilities?
Thanks
-neil
On 6/26/2014 10:57 AM, Zhengyu Gu wrote:
Hi Neil,
There is still an issue. Apparently, you can NOT free the buffer for the environment variable.
678 char * pbuf = (char*)JLI_MemAlloc(pbuflen); 679 JLI_Snprintf(pbuf, pbuflen, "%s%d=%s", NMT_Env_Name, JLI_GetPid(), value); 680 retval = JLI_PutEnv(pbuf); 681 if (JLI_IsTraceLauncher()) { 682 char* envName; 683 char* envBuf; 684 685 // ensures that malloc successful 686 envName = (char*)JLI_MemAlloc(pbuflen); 687 JLI_Snprintf(envName, pbuflen, "%s%d", NMT_Env_Name, JLI_GetPid()); 688 689 printf("TRACER_MARKER: NativeMemoryTracking: env var is %s\n",envName); 690 printf("TRACER_MARKER: NativeMemoryTracking: putenv arg %s\n",pbuf); 691 envBuf = getenv(envName); 692 printf("TRACER_MARKER: NativeMemoryTracking: got value %s\n",envBuf); 693 free(envName); 694 } 695 696 free(pbuf); <<<<=== can not free this buffer 697 }
You can experiment it move #696 to #681, you will see the test to fail.
Linux putenv document says:
*putenv*is very widely available, but it might or might not copy its argument, risking memory leaks.
Thanks,
-Zhengyu
On 6/25/2014 4:40 PM, Neil Toda wrote:
Okay, Very glad you checked. It really does need to go as early as your prototype suggested. I'll get right on it.
-neil
On 6/25/2014 12:05 PM, Zhengyu Gu wrote:
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 >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
Hi Zhengyu, You are right then this needs to happen before LoadJavaVM, I think that is where it was initially, but I had Neil move it to ParseArguments where all the arguments are parsed. Also it is probably best that you push this changeset when it is complete, into hotspot-dev, along with the JVM changes. Kumar On 6/25/2014 12:05 PM, Zhengyu Gu wrote:
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 >>>>> >>>> >>> >> >
Zhengyu Earlier creation of the environmental variable. http://cr.openjdk.java.net/~ntoda/8042469/webrev-07/ The new webrev [07] places the new code in the function SetJvmEnvironment() === . JLI_Launch() . InitLauncher() . CreateExecutionEnvironment() . CheckJvmType() . SetJvmEnvironment() . LoadJavaVM() ... . ParseArguments() === Kumar and I settled on this organization last night, moving the code out of ParseArguments() into its own function. All previous review comments for the launcher and test are included in this revision. Thanks -neil On 6/25/2014 12:05 PM, Zhengyu Gu wrote:
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 >>>>> >>>> >>> >> >
Looks fine. Kumar On 6/26/2014 8:12 AM, Neil Toda wrote:
Zhengyu
Earlier creation of the environmental variable.
http://cr.openjdk.java.net/~ntoda/8042469/webrev-07/
The new webrev [07] places the new code in the function SetJvmEnvironment()
=== . JLI_Launch() . InitLauncher() . CreateExecutionEnvironment() . CheckJvmType() . SetJvmEnvironment() . LoadJavaVM() ... . ParseArguments() ===
Kumar and I settled on this organization last night, moving the code out of ParseArguments() into its own function.
All previous review comments for the launcher and test are included in this revision.
Thanks
-neil
On 6/25/2014 12:05 PM, Zhengyu Gu wrote:
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 >>>>>> >>>>> >>>> >>> >> >
participants (5)
-
Joe Darcy
-
Kumar Srinivasan
-
Neil Toda
-
Vitaly Davidovich
-
Zhengyu Gu