Ready for Review : 8042469 : Launcher changes for native memory tracking scalability enhancement
Kumar Srinivasan
kumar.x.srinivasan at oracle.com
Fri Jun 27 12:50:38 UTC 2014
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
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>
>
More information about the core-libs-dev
mailing list