From c5af85ecadb8149ddb06ae473b47ca783e018eb0 Mon Sep 17 00:00:00 2001 From: Adam Retter Date: Thu, 4 Feb 2016 14:28:18 +0000 Subject: [PATCH] Fix a memory leak of Slice objects from org.rocksdb.WBWIRocksIterator#entry1 --- java/rocksjni/comparatorjnicallback.cc | 6 +- java/rocksjni/portal.h | 84 +++---------------- java/rocksjni/write_batch_with_index.cc | 46 ++++++++-- .../main/java/org/rocksdb/AbstractSlice.java | 26 +++--- .../main/java/org/rocksdb/DirectSlice.java | 21 +++-- .../java/org/rocksdb/RocksMutableObject.java | 43 +++++++--- java/src/main/java/org/rocksdb/Slice.java | 5 +- .../java/org/rocksdb/WBWIRocksIterator.java | 43 +++++++--- 8 files changed, 141 insertions(+), 133 deletions(-) diff --git a/java/rocksjni/comparatorjnicallback.cc b/java/rocksjni/comparatorjnicallback.cc index 1c0317003c..57ee0f95cf 100644 --- a/java/rocksjni/comparatorjnicallback.cc +++ b/java/rocksjni/comparatorjnicallback.cc @@ -60,8 +60,8 @@ int BaseComparatorJniCallback::Compare(const Slice& a, const Slice& b) const { // performance. mtx_compare->Lock(); - AbstractSliceJni::setHandle(m_env, m_jSliceA, &a); - AbstractSliceJni::setHandle(m_env, m_jSliceB, &b); + AbstractSliceJni::setHandle(m_env, m_jSliceA, &a, JNI_FALSE); + AbstractSliceJni::setHandle(m_env, m_jSliceB, &b, JNI_FALSE); jint result = m_env->CallIntMethod(m_jComparator, m_jCompareMethodId, m_jSliceA, m_jSliceB); @@ -89,7 +89,7 @@ void BaseComparatorJniCallback::FindShortestSeparator( // performance. mtx_findShortestSeparator->Lock(); - AbstractSliceJni::setHandle(m_env, m_jSliceLimit, &limit); + AbstractSliceJni::setHandle(m_env, m_jSliceLimit, &limit, JNI_FALSE); jstring jsResultStart = (jstring)m_env->CallObjectMethod(m_jComparator, m_jFindShortestSeparatorMethodId, jsStart, m_jSliceLimit); diff --git a/java/rocksjni/portal.h b/java/rocksjni/portal.h index ef7df88376..f4ad29af5a 100644 --- a/java/rocksjni/portal.h +++ b/java/rocksjni/portal.h @@ -49,25 +49,22 @@ template class RocksDBNativeClass { assert(jclazz != nullptr); return jclazz; } - - // Get the field id of the member variable to store - // the ptr - static jfieldID getHandleFieldID(JNIEnv* env) { - static jfieldID fid = env->GetFieldID( - DERIVED::getJClass(env), "nativeHandle_", "J"); - assert(fid != nullptr); - return fid; - } }; // Native class template for sub-classes of RocksMutableObject template class NativeRocksMutableObject : public RocksDBNativeClass { public: + + static jmethodID getSetNativeHandleMethod(JNIEnv* env) { + static jmethodID mid = env->GetMethodID( + DERIVED::getJClass(env), "setNativeHandle", "(JZ)V"); + assert(mid != nullptr); + return mid; + } + // Pass the pointer to the java side. - static void setHandle(JNIEnv* env, jobject jdb, PTR ptr) { - env->SetLongField( - jdb, RocksDBNativeClass::getHandleFieldID(env), - reinterpret_cast(ptr)); + static void setHandle(JNIEnv* env, jobject jobj, PTR ptr, jboolean java_owns_handle) { + env->CallVoidMethod(jobj, getSetNativeHandleMethod(env), reinterpret_cast(ptr), java_owns_handle); } }; @@ -647,67 +644,6 @@ class WriteEntryJni { assert(jclazz != nullptr); return jclazz; } - - static void setWriteType(JNIEnv* env, jobject jwrite_entry, - WriteType write_type) { - jobject jwrite_type; - switch (write_type) { - case kPutRecord: - jwrite_type = WriteTypeJni::PUT(env); - break; - - case kMergeRecord: - jwrite_type = WriteTypeJni::MERGE(env); - break; - - case kDeleteRecord: - jwrite_type = WriteTypeJni::DELETE(env); - break; - - case kLogDataRecord: - jwrite_type = WriteTypeJni::LOG(env); - break; - - default: - jwrite_type = nullptr; - } - assert(jwrite_type != nullptr); - env->SetObjectField(jwrite_entry, getWriteTypeField(env), jwrite_type); - } - - static void setKey(JNIEnv* env, jobject jwrite_entry, - const rocksdb::Slice* slice) { - jobject jkey = env->GetObjectField(jwrite_entry, getKeyField(env)); - AbstractSliceJni::setHandle(env, jkey, slice); - } - - static void setValue(JNIEnv* env, jobject jwrite_entry, - const rocksdb::Slice* slice) { - jobject jvalue = env->GetObjectField(jwrite_entry, getValueField(env)); - AbstractSliceJni::setHandle(env, jvalue, slice); - } - - private: - static jfieldID getWriteTypeField(JNIEnv* env) { - static jfieldID fid = env->GetFieldID( - getJClass(env), "type", "Lorg/rocksdb/WBWIRocksIterator$WriteType;"); - assert(fid != nullptr); - return fid; - } - - static jfieldID getKeyField(JNIEnv* env) { - static jfieldID fid = env->GetFieldID( - getJClass(env), "key", "Lorg/rocksdb/DirectSlice;"); - assert(fid != nullptr); - return fid; - } - - static jfieldID getValueField(JNIEnv* env) { - static jfieldID fid = env->GetFieldID( - getJClass(env), "value", "Lorg/rocksdb/DirectSlice;"); - assert(fid != nullptr); - return fid; - } }; class InfoLogLevelJni { diff --git a/java/rocksjni/write_batch_with_index.cc b/java/rocksjni/write_batch_with_index.cc index ade91b63a7..1c7b6711c7 100644 --- a/java/rocksjni/write_batch_with_index.cc +++ b/java/rocksjni/write_batch_with_index.cc @@ -353,27 +353,57 @@ void Java_org_rocksdb_WBWIRocksIterator_status0( /* * Class: org_rocksdb_WBWIRocksIterator * Method: entry1 - * Signature: (JLorg/rocksdb/WBWIRocksIterator/WriteEntry;)V + * Signature: (J)[J */ -void Java_org_rocksdb_WBWIRocksIterator_entry1( - JNIEnv* env, jobject jobj, jlong handle, jobject jwrite_entry) { +jlongArray Java_org_rocksdb_WBWIRocksIterator_entry1( + JNIEnv* env, jobject jobj, jlong handle) { auto* it = reinterpret_cast(handle); const rocksdb::WriteEntry& we = it->Entry(); - jobject jwe = rocksdb::WBWIRocksIteratorJni::getWriteEntry(env, jobj); - rocksdb::WriteEntryJni::setWriteType(env, jwe, we.type); + jlong results[3]; + + //set the type of the write entry + switch (we.type) { + case rocksdb::kPutRecord: + results[0] = 0x1; + break; + + case rocksdb::kMergeRecord: + results[0] = 0x2; + break; + + case rocksdb::kDeleteRecord: + results[0] = 0x4; + break; + + case rocksdb::kLogDataRecord: + results[0] = 0x8; + break; + + default: + results[0] = 0x0; + } + + //TODO(AR) do we leak buf and value_buf? + + //set the pointer to the key slice char* buf = new char[we.key.size()]; memcpy(buf, we.key.data(), we.key.size()); auto* key_slice = new rocksdb::Slice(buf, we.key.size()); - rocksdb::WriteEntryJni::setKey(env, jwe, key_slice); + results[1] = reinterpret_cast(key_slice); + //set the pointer to the value slice if (we.type == rocksdb::kDeleteRecord || we.type == rocksdb::kLogDataRecord) { // set native handle of value slice to null if no value available - rocksdb::WriteEntryJni::setValue(env, jwe, nullptr); + results[2] = 0; } else { char* value_buf = new char[we.value.size()]; memcpy(value_buf, we.value.data(), we.value.size()); auto* value_slice = new rocksdb::Slice(value_buf, we.value.size()); - rocksdb::WriteEntryJni::setValue(env, jwe, value_slice); + results[2] = reinterpret_cast(value_slice); } + + jlongArray jresults = env->NewLongArray(3); + env->SetLongArrayRegion(jresults, 0, 3, results); + return jresults; } diff --git a/java/src/main/java/org/rocksdb/AbstractSlice.java b/java/src/main/java/org/rocksdb/AbstractSlice.java index b9e67d4727..c5fd2f58cc 100644 --- a/java/src/main/java/org/rocksdb/AbstractSlice.java +++ b/java/src/main/java/org/rocksdb/AbstractSlice.java @@ -42,8 +42,7 @@ abstract class AbstractSlice extends RocksMutableObject { * @see org.rocksdb.AbstractSlice#data0(long) */ public T data() { - assert (isOwningHandle()); - return data0(nativeHandle_); + return data0(getNativeHandle()); } /** @@ -64,8 +63,7 @@ abstract class AbstractSlice extends RocksMutableObject { * @return The length in bytes. */ public int size() { - assert (isOwningHandle()); - return size0(nativeHandle_); + return size0(getNativeHandle()); } /** @@ -75,8 +73,7 @@ abstract class AbstractSlice extends RocksMutableObject { * @return true if there is no data, false otherwise. */ public boolean empty() { - assert (isOwningHandle()); - return empty0(nativeHandle_); + return empty0(getNativeHandle()); } /** @@ -88,8 +85,7 @@ abstract class AbstractSlice extends RocksMutableObject { * @return The string representation of the data. */ public String toString(final boolean hex) { - assert (isOwningHandle()); - return toString0(nativeHandle_, hex); + return toString0(getNativeHandle(), hex); } @Override @@ -109,8 +105,15 @@ abstract class AbstractSlice extends RocksMutableObject { */ public int compare(final AbstractSlice other) { assert (other != null); - assert (isOwningHandle()); - return compare0(nativeHandle_, other.nativeHandle_); + if(!isOwningHandle()) { + return other.isOwningHandle() ? -1 : 0; + } else { + if(!other.isOwningHandle()) { + return 1; + } else { + return compare0(getNativeHandle(), other.getNativeHandle()); + } + } } @Override @@ -149,8 +152,7 @@ abstract class AbstractSlice extends RocksMutableObject { */ public boolean startsWith(final AbstractSlice prefix) { if (prefix != null) { - assert (isOwningHandle()); - return startsWith0(nativeHandle_, prefix.nativeHandle_); + return startsWith0(getNativeHandle(), prefix.getNativeHandle()); } else { return false; } diff --git a/java/src/main/java/org/rocksdb/DirectSlice.java b/java/src/main/java/org/rocksdb/DirectSlice.java index 9f82691051..8f96eb49f6 100644 --- a/java/src/main/java/org/rocksdb/DirectSlice.java +++ b/java/src/main/java/org/rocksdb/DirectSlice.java @@ -24,10 +24,11 @@ public class DirectSlice extends AbstractSlice { * at creation time. * * Note: You should be aware that it is intentionally marked as - * package-private. This is so that developers cannot construct their own default - * DirectSlice objects (at present). As developers cannot construct their own - * DirectSlice objects through this, they are not creating underlying C++ - * DirectSlice objects, and so there is nothing to free (dispose) from Java. + * package-private. This is so that developers cannot construct their own + * default DirectSlice objects (at present). As developers cannot construct + * their own DirectSlice objects through this, they are not creating + * underlying C++ DirectSlice objects, and so there is nothing to free + * (dispose) from Java. */ DirectSlice() { super(); @@ -68,7 +69,8 @@ public class DirectSlice extends AbstractSlice { } private static ByteBuffer ensureDirect(final ByteBuffer data) { - //TODO(AR) consider throwing a checked exception, as if it's not direct this can SIGSEGV + // TODO(AR) consider throwing a checked exception, as if it's not direct + // this can SIGSEGV assert(data.isDirect()); return data; } @@ -82,16 +84,14 @@ public class DirectSlice extends AbstractSlice { * @return the requested byte */ public byte get(int offset) { - assert (isOwningHandle()); - return get0(nativeHandle_, offset); + return get0(getNativeHandle(), offset); } /** * Clears the backing slice */ public void clear() { - assert (isOwningHandle()); - clear0(nativeHandle_); + clear0(getNativeHandle()); } /** @@ -102,8 +102,7 @@ public class DirectSlice extends AbstractSlice { * @param n The number of bytes to drop */ public void removePrefix(final int n) { - assert (isOwningHandle()); - removePrefix0(nativeHandle_, n); + removePrefix0(getNativeHandle(), n); } private native static long createNewDirectSlice0(final ByteBuffer data, diff --git a/java/src/main/java/org/rocksdb/RocksMutableObject.java b/java/src/main/java/org/rocksdb/RocksMutableObject.java index f4ca3565d1..823711742f 100644 --- a/java/src/main/java/org/rocksdb/RocksMutableObject.java +++ b/java/src/main/java/org/rocksdb/RocksMutableObject.java @@ -1,30 +1,47 @@ package org.rocksdb; -public abstract class RocksMutableObject extends NativeReference { +public abstract class RocksMutableObject /*extends NativeReference*/ { - private final boolean shouldOwnHandle; - protected volatile long nativeHandle_; + private long nativeHandle_; + private boolean owningHandle_; protected RocksMutableObject() { - super(false); - this.shouldOwnHandle = false; } protected RocksMutableObject(final long nativeHandle) { - super(true); - this.shouldOwnHandle = true; this.nativeHandle_ = nativeHandle; + this.owningHandle_ = true; + } + + public synchronized void setNativeHandle(final long nativeHandle, final boolean owningNativeHandle) { + this.nativeHandle_ = nativeHandle; + this.owningHandle_ = owningNativeHandle; + } + + //@Override + protected synchronized boolean isOwningHandle() { + return this.owningHandle_; + } + + protected synchronized long getNativeHandle() { + assert(this.nativeHandle_ != 0); + return this.nativeHandle_; + } + + public synchronized final void dispose() { + if(isOwningHandle()) { + disposeInternal(); + this.owningHandle_ = false; + this.nativeHandle_ = 0; + } } @Override - public boolean isOwningHandle() { - return ((!shouldOwnHandle) || super.isOwningHandle()) && nativeHandle_ != 0; + protected void finalize() throws Throwable { + dispose(); + super.finalize(); } - /** - * Deletes underlying C++ object pointer. - */ - @Override protected void disposeInternal() { disposeInternal(nativeHandle_); } diff --git a/java/src/main/java/org/rocksdb/Slice.java b/java/src/main/java/org/rocksdb/Slice.java index ae0815392f..cbb9742a37 100644 --- a/java/src/main/java/org/rocksdb/Slice.java +++ b/java/src/main/java/org/rocksdb/Slice.java @@ -73,8 +73,9 @@ public class Slice extends AbstractSlice { */ @Override protected void disposeInternal() { - disposeInternalBuf(nativeHandle_); - super.disposeInternal(); + final long nativeHandle = getNativeHandle(); + disposeInternalBuf(nativeHandle); + super.disposeInternal(nativeHandle); } @Override protected final native byte[] data0(long handle); diff --git a/java/src/main/java/org/rocksdb/WBWIRocksIterator.java b/java/src/main/java/org/rocksdb/WBWIRocksIterator.java index 6d06c8bd34..7eb019d0b2 100644 --- a/java/src/main/java/org/rocksdb/WBWIRocksIterator.java +++ b/java/src/main/java/org/rocksdb/WBWIRocksIterator.java @@ -5,10 +5,12 @@ package org.rocksdb; -public class WBWIRocksIterator extends AbstractRocksIterator { +public class WBWIRocksIterator + extends AbstractRocksIterator { private final WriteEntry entry = new WriteEntry(); - protected WBWIRocksIterator(final WriteBatchWithIndex wbwi, final long nativeHandle) { + protected WBWIRocksIterator(final WriteBatchWithIndex wbwi, + final long nativeHandle) { super(wbwi, nativeHandle); } @@ -20,12 +22,20 @@ public class WBWIRocksIterator extends AbstractRocksIterator