Ready for Review : 8042469 : Launcher changes for native memory tracking scalability enhancement
Zhengyu Gu
zhengyu.gu at oracle.com
Thu Jun 26 20:09:56 UTC 2014
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+call+putenv()+with+a+pointer+to+an+automatic+variable+as+the+argument
>
> 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
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the core-libs-dev
mailing list