On 29.04.2015 17:03, Branko Čibej wrote: > On 29.04.2015 16:02, Branko Čibej wrote: >> 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.) > > So, interesting data point ... I moved the creation of the Checksum > objects after the creation of the property maps ... and now they're > getting garbage-collected. This is becoming extremely weird.
Hah. Fixed it. http://svn.apache.org/r1676771 We were not properly popping off a JNI frame in CreateJ::PropertyMap, so the checksum objects were referenced in a JNI frame that was left hanging in limbo after the call to finishReport returned. I'm running your test case now (with -Xmx24M), currently at over 350 iterations with total memory usage stable at around 50MB with no forced garbage collections. -- Brane
