On 29.04.2015 11:57, Marc Strapetz wrote: > On 29.04.2015 05:31, Branko Čibej wrote: >> On 28.04.2015 21:22, Bert Huijben wrote: >>> >>>> -----Original Message----- >>>> From: Marc Strapetz [mailto:marc.strap...@syntevo.com] >>>> Sent: dinsdag 28 april 2015 20:26 >>>> To: Branko Čibej >>>> Cc: Subversion Development >>>> Subject: Re: 1.9 JavaHL memory leak in ISVNRemote#status >>>> Also, I should add that according to the Profiler, the byte[]s are >>>> referenced from the Checksums. The char[]s are referenced from the >>>> Strings. And the Strings are referenced directly as JNI local >>>> references. Browsing through these Strings, they seem to be >>>> server-side >>>> paths ("subversion/branches/1.8.x/...") >>> Just guessing: Notifications? >> >> >> No, this is an RA status edit drive; there are no notifications, only >> editor callbacks, and the checksum objects are created in in the >> callbacks related to file content changes (file contents streams and >> checksums always come in pairs). >> >> I counted creations, finalizations and garbage collections again. I >> added forced finalization and GC calls to the test case. For every loop >> in the test, we create 57 Checksum instances, but only one of them is >> finalized, no matter how often the finalizer and GC are run. All the >> Checksum objects are created in the same way, and here are /no/ >> references anywhere to the remaining 56 objects, yet they're neither >> finalized nor garbage-collected. The fields (byte array and kind) /are/ >> collected; all the "live" (according to the heap profiler) Checksum >> objects have their fields set to null. > > I've been testing on Windows. According to JProfiler and JVisualVM, > byte[]s are still referenced from the Checksums. Hence, I would expect > that they are not garbage collected. > >> clearly, the code is cleaning up the references correctly > > I don't have detailed understanding of the "jniwrapper" package, but I > tend to agree with you. In the native code, CreateJ::Checksum and > CreateJ::PropertyMap are basically doing the same thing, so there is > no reason why Checksums would remain referenced while HashMaps > properly do not. > > I've also tried to comment out all env.CallVoidMethod()-callbacks in > EditorProxy.cpp, so created object references would not even be passed > into the Java code. Still the same, Checksums remain as "JNI local > reference". > > Finally, I've tried to explicitly call DeleteLocalRef(). This /solves/ > the memory leak (at least for Checksums), but I don't understand why > this is necessary and whether this is correct. > > svn_error_t* > EditorProxy::cb_alter_file(void *baton, > const char *relpath, > ... > jstring jrelpath = JNIUtil::makeJString(relpath); > SVN_JAVAHL_OLDSTYLE_EXCEPTION_CHECK(env); > jobject jchecksum = CreateJ::Checksum(checksum); > SVN_JAVAHL_OLDSTYLE_EXCEPTION_CHECK(env); > jobject jprops = CreateJ::PropertyMap(props, scratch_pool); > SVN_JAVAHL_OLDSTYLE_EXCEPTION_CHECK(env); > > jobject jcontents = NULL; > if (contents != NULL) > jcontents = wrap_input_stream(contents); > > env.CallVoidMethod(ep->m_jeditor, mid, > jrelpath, jlong(revision), > jchecksum, jcontents, jprops); > > env.DeleteLocalRef(jrelpath); > env.DeleteLocalRef(jchecksum); > env.DeleteLocalRef(jprops); > if (contents != NULL) > env.DeleteLocalRef(jcontents); > ... > >> but for some unfathomable reason, the collector keeps them >> alive for a while. > > I'm not entirely sure about the exact difference of the live data in > the VM and a heap dump, but IMO the Checksums are still considered as > referenced ("JNI local reference") and hence will never be garbage > collected. The profilers confirm this. > > Given that DeleteLocalRef solves the problem, I think this is either a > bug in the jniwrapper or a bug in JNI itself.
The latest code wraps the callback implementations with PushLocalFrame/PopLocalFrame; any references created within a local frame should be automatically deleted by PopLocalFrame, according to all JNI docs I can find. I can add the explicit deletions, but it's a shame that frame management wouldn't work as expected. :( So, I'm going to double-check if we're actually getting the frame management right. I can't imagine why the HashMaps and NativeInputStreams would be released, but not the Checksums. All in all, I agree with you that this looks like a JNI bug ... the trick now will be to prove that with a minimal test case and report it upstream. :) (FWIW, I'm using Java 8u45 64-bit on OSX.) -- Brane
