RR(S): 8020943: Memory leak when GCNotifier uses create_from_platform_dependent_str()

[Kevin Walls]

On 31/07/13 15:59, Kevin Walls wrote:
>
> On 31/07/13 14:44, Daniel D. Daugherty wrote:
>> On 7/31/13 3:45 AM, Kevin Walls wrote:
>>> Hi,
>>>
>>> I'd like to get a review on this small change:
>>>
>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8020943
>>> https://jbs.oracle.com/bugs/browse/JDK-8020943
>>>
>>> webrev
>>> http://cr.openjdk.java.net/~kevinw/8020943/webrev.00/
>>
>> src/share/vm/services/gcNotifier.cpp
>>     No comments.
>>
>> The fix is fine in that you changed the code to call a function that
>> doesn't leak a JNI handle, but the original JNI handle leak that you
>> found in java_lang_String::create_from_platform_dependent_str()
>> remains right?
>>
>> I think create_from_platform_dependent_str() still needs the call to
>> "JNIHandles::destroy_local(js)". The next function down in the same
>> file, java_lang_String::as_platform_dependent_str(), takes care to
>> call "JNIHandles::destroy_local(js)" for its locally created JNI
>> handle so there's good precedent for cleaning up such things.
>>
>> Dan
>>
>
>
> Hi Dan,
>
> Yes.  It was also suggested to me that the JNIHandle would be cleared 
> on a transition back to Java (which never happens in this case), so 
> although I don't see many other callers, they may not all see a problem.
>
> In another thread my initial thought was in 
> create_from_platform_dependent_str itself, to clear the JNIHandle.  I 
> just suggested the simple function-name-change here as all the jni 
> work is unnecessary for what the gcNotifier needs.
>
> Maybe I should make both changes, including:
>
> $ hg diff  ../src/share/vm/classfile/javaClasses.cpp
> diff --git a/src/share/vm/classfile/javaClasses.cpp
> b/src/share/vm/classfile/javaClasses.cpp
> --- a/src/share/vm/classfile/javaClasses.cpp
> +++ b/src/share/vm/classfile/javaClasses.cpp
> @@ -244,7 +244,10 @@
>       ThreadToNativeFromVM ttn(thread);
>       js = (_to_java_string_fn)(thread->jni_environment(), str);
>     }
> -  return Handle(THREAD, JNIHandles::resolve(js));
> +  // XXX KJW we get a jstring back, but leak it...
> +  Handle result = Handle(THREAD, JNIHandles::resolve(js));
> +  JNIHandles::destroy_local(js);
> +  return result;
>   }
>
>
> Thanks
> Kevin
>
>
>
>
>
>>
>>>
>>> It turns out there's a leak in the gc notifier: reproduce by 
>>> attaching e.g. JConsole and watching, if there is frequent GC the 
>>> number of apparently unowned String objects that can't get collect 
>>> continually increases.
>>>
>>> In the notifier, the method it calls to create String objects 
>>> involves a JNI call that leaves a Handle behind and doesn't get 
>>> cleared.   There is a simpler method to call, there is no need for 
>>> all that work, as we are dealing with a small set of simple strings 
>>> in the JVM being converted, to describe the collection type, cause, ...
>>>
>>> Thanks
>>> Kevin
>>
>


Thinking about it a few seconds more, we should likely do both things, 
so I'm proposing splitting this up - push 8020943 as per the webrev 
here, considering the work of the JNI method is unnecessary for the 
gcnotifier, but follow up with another CR to remove the handle like in 
the diff I mention in that last reply (separate review to follow...).

Both are good things, but rolling them in together is unnecessary, and 
if there's any impact of changing the handle behaviour in the string 
creation method, then having it in a separate push from fixing the leak 
seems good.

Thanks
Kevin