Ready for Review : 8042469 : Launcher changes for native memory tracking scalability enhancement

[Zhengyu Gu]

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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>>
>>
>