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.strapetz_at_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
Received on 2015-04-29 16:03:34 CEST