On 29.04.2015 19:44, Philip Martin wrote:
> brane_at_apache.org writes:
>
>> Author: brane
>> Date: Wed Apr 29 15:30:54 2015
>> New Revision: 1676769
>>
>> URL: http://svn.apache.org/r1676769
>> Log:
>> Fix another silly memory leak in JavaHL.
>> This time, we forgot to close off a JNI frame.
>> - m_env->CallObjectMethod(m_map, m_put_mid, jpropName, jpropVal);
>> + jobject ret = m_env->CallObjectMethod(m_map, m_put_mid,
>> + jpropName, jpropVal);
>> if (JNIUtil::isJavaExceptionThrown())
>> return;
>>
>> - m_env->DeleteLocalRef(jpropName);
>> + m_env->DeleteLocalRef(ret);
> Should we do the same to the other places we use CallObjectMethod
> without calling DeleteLocalRef? As I understand it the local ref
> "leaks" are temporary and only really matter if we create them in some
> sort of loop. So perhaps
>
> CreateJ::LockMap
> RemoteSession::getMergeinfo (and the fill_dirents and
> location_hash_to_map functions)
Yes, to minimize the overflow of local references within a loop, we
definitely should do that. Not because there's an actual leak in the
cases you point out, but to minimize the size of the JNI reference table.
I just don't think we should bother with such cosmetic changes now
without also switching the code to the new-style wrappers.
The more important part of this change was the addition of
POP_AND_RETURN_NOTHING at the end of the function (that you don't
quote), because that actually destroys ("pops") the current local JNI
frame and cleans up any remaining local references within it. We were
leaking a whole frame, not just the single object reference in the loop,
and that was affecting other code that happened to be in proximity to
the call sites of this function.
-- Brane
Received on 2015-04-29 22:49:33 CEST