From de678b288e36d0403d76d278fac9f6c97d08c44c Mon Sep 17 00:00:00 2001 From: Adam Retter Date: Sat, 3 Jan 2015 15:08:10 +0000 Subject: [PATCH] Abstractions for common iterator behaviour --- java/org/rocksdb/AbstractRocksIterator.java | 105 ++++++++++++++++++++ java/org/rocksdb/RocksIterator.java | 89 ++--------------- java/rocksjni/iterator.cc | 88 ++++++++-------- 3 files changed, 160 insertions(+), 122 deletions(-) create mode 100644 java/org/rocksdb/AbstractRocksIterator.java diff --git a/java/org/rocksdb/AbstractRocksIterator.java b/java/org/rocksdb/AbstractRocksIterator.java new file mode 100644 index 000000000..cc7cf064f --- /dev/null +++ b/java/org/rocksdb/AbstractRocksIterator.java @@ -0,0 +1,105 @@ +// Copyright (c) 2014, Facebook, Inc. All rights reserved. +// This source code is licensed under the BSD-style license found in the +// LICENSE file in the root directory of this source tree. An additional grant +// of patent rights can be found in the PATENTS file in the same directory. + +package org.rocksdb; + +/** + * Base class implementation for Rocks Iterators + * in the Java API + *

+ *

Multiple threads can invoke const methods on an RocksIterator without + * external synchronization, but if any of the threads may call a + * non-const method, all threads accessing the same RocksIterator must use + * external synchronization.

+ * + * @param P The type of the Parent Object from which the Rocks Iterator was + * created. This is used by disposeInternal to avoid double-free + * issues with the underlying C++ object. + * @see org.rocksdb.RocksObject + */ +public abstract class AbstractRocksIterator

+ extends RocksObject implements RocksIteratorInterface { + final P parent_; + + protected AbstractRocksIterator(P parent, long nativeHandle) { + super(); + nativeHandle_ = nativeHandle; + // parent must point to a valid RocksDB instance. + assert (parent != null); + // RocksIterator must hold a reference to the related parent instance + // to guarantee that while a GC cycle starts RocksIterator instances + // are freed prior to parent instances. + parent_ = parent; + } + + @Override + public boolean isValid() { + assert (isInitialized()); + return isValid0(nativeHandle_); + } + + @Override + public void seekToFirst() { + assert (isInitialized()); + seekToFirst0(nativeHandle_); + } + + @Override + public void seekToLast() { + assert (isInitialized()); + seekToLast0(nativeHandle_); + } + + @Override + public void seek(byte[] target) { + assert (isInitialized()); + seek0(nativeHandle_, target, target.length); + } + + @Override + public void next() { + assert (isInitialized()); + next0(nativeHandle_); + } + + @Override + public void prev() { + assert (isInitialized()); + prev0(nativeHandle_); + } + + @Override + public void status() throws RocksDBException { + assert (isInitialized()); + status0(nativeHandle_); + } + + /** + *

Deletes underlying C++ iterator pointer.

+ *

+ *

Note: the underlying handle can only be safely deleted if the parent + * instance related to a certain RocksIterator is still valid and initialized. + * Therefore {@code disposeInternal()} checks if the parent is initialized + * before freeing the native handle.

+ */ + @Override + protected void disposeInternal() { + synchronized (parent_) { + assert (isInitialized()); + if (parent_.isInitialized()) { + disposeInternal(nativeHandle_); + } + } + } + + abstract void disposeInternal(long handle); + abstract boolean isValid0(long handle); + abstract void seekToFirst0(long handle); + abstract void seekToLast0(long handle); + abstract void next0(long handle); + abstract void prev0(long handle); + abstract void seek0(long handle, byte[] target, int targetLen); + abstract void status0(long handle) throws RocksDBException; +} diff --git a/java/org/rocksdb/RocksIterator.java b/java/org/rocksdb/RocksIterator.java index cecf9c309..bb9a6e697 100644 --- a/java/org/rocksdb/RocksIterator.java +++ b/java/org/rocksdb/RocksIterator.java @@ -18,58 +18,9 @@ package org.rocksdb; * * @see org.rocksdb.RocksObject */ -public class RocksIterator extends RocksObject implements RocksIteratorInterface { - public RocksIterator(RocksDB rocksDB, long nativeHandle) { - super(); - nativeHandle_ = nativeHandle; - // rocksDB must point to a valid RocksDB instance. - assert(rocksDB != null); - // RocksIterator must hold a reference to the related RocksDB instance - // to guarantee that while a GC cycle starts RocksDBIterator instances - // are freed prior to RocksDB instances. - rocksDB_ = rocksDB; - } - - @Override - public boolean isValid() { - assert(isInitialized()); - return isValid0(nativeHandle_); - } - - @Override - public void seekToFirst() { - assert(isInitialized()); - seekToFirst0(nativeHandle_); - } - - @Override - public void seekToLast() { - assert(isInitialized()); - seekToLast0(nativeHandle_); - } - - @Override - public void seek(byte[] target) { - assert(isInitialized()); - seek0(nativeHandle_, target, target.length); - } - - @Override - public void next() { - assert(isInitialized()); - next0(nativeHandle_); - } - - @Override - public void prev() { - assert(isInitialized()); - prev0(nativeHandle_); - } - - @Override - public void status() throws RocksDBException { - assert(isInitialized()); - status0(nativeHandle_); +public class RocksIterator extends AbstractRocksIterator { + protected RocksIterator(RocksDB rocksDB, long nativeHandle) { + super(rocksDB, nativeHandle); } /** @@ -99,33 +50,15 @@ public class RocksIterator extends RocksObject implements RocksIteratorInterface return value0(nativeHandle_); } - /** - *

Deletes underlying C++ iterator pointer.

- * - *

Note: the underlying handle can only be safely deleted if the RocksDB - * instance related to a certain RocksIterator is still valid and initialized. - * Therefore {@code disposeInternal()} checks if the RocksDB is initialized - * before freeing the native handle.

- */ - @Override protected void disposeInternal() { - synchronized (rocksDB_) { - assert (isInitialized()); - if (rocksDB_.isInitialized()) { - disposeInternal(nativeHandle_); - } - } - } + @Override final native void disposeInternal(long handle); + @Override final native boolean isValid0(long handle); + @Override final native void seekToFirst0(long handle); + @Override final native void seekToLast0(long handle); + @Override final native void next0(long handle); + @Override final native void prev0(long handle); + @Override final native void seek0(long handle, byte[] target, int targetLen); + @Override final native void status0(long handle) throws RocksDBException; - private native boolean isValid0(long handle); - private native void disposeInternal(long handle); - private native void seekToFirst0(long handle); - private native void seekToLast0(long handle); - private native void next0(long handle); - private native void prev0(long handle); private native byte[] key0(long handle); private native byte[] value0(long handle); - private native void seek0(long handle, byte[] target, int targetLen); - private native void status0(long handle); - - final RocksDB rocksDB_; } diff --git a/java/rocksjni/iterator.cc b/java/rocksjni/iterator.cc index c7667a018..e9eb0bb37 100644 --- a/java/rocksjni/iterator.cc +++ b/java/rocksjni/iterator.cc @@ -14,6 +14,17 @@ #include "rocksjni/portal.h" #include "rocksdb/iterator.h" +/* + * Class: org_rocksdb_RocksIterator + * Method: disposeInternal + * Signature: (J)V + */ +void Java_org_rocksdb_RocksIterator_disposeInternal( + JNIEnv* env, jobject jobj, jlong handle) { + auto it = reinterpret_cast(handle); + delete it; +} + /* * Class: org_rocksdb_RocksIterator * Method: isValid0 @@ -36,7 +47,7 @@ void Java_org_rocksdb_RocksIterator_seekToFirst0( /* * Class: org_rocksdb_RocksIterator - * Method: seekToFirst0 + * Method: seekToLast0 * Signature: (J)V */ void Java_org_rocksdb_RocksIterator_seekToLast0( @@ -46,7 +57,7 @@ void Java_org_rocksdb_RocksIterator_seekToLast0( /* * Class: org_rocksdb_RocksIterator - * Method: seekToLast0 + * Method: next0 * Signature: (J)V */ void Java_org_rocksdb_RocksIterator_next0( @@ -56,7 +67,7 @@ void Java_org_rocksdb_RocksIterator_next0( /* * Class: org_rocksdb_RocksIterator - * Method: next0 + * Method: prev0 * Signature: (J)V */ void Java_org_rocksdb_RocksIterator_prev0( @@ -66,41 +77,8 @@ void Java_org_rocksdb_RocksIterator_prev0( /* * Class: org_rocksdb_RocksIterator - * Method: prev0 - * Signature: (J)V - */ -jbyteArray Java_org_rocksdb_RocksIterator_key0( - JNIEnv* env, jobject jobj, jlong handle) { - auto it = reinterpret_cast(handle); - rocksdb::Slice key_slice = it->key(); - - jbyteArray jkey = env->NewByteArray(static_cast(key_slice.size())); - env->SetByteArrayRegion(jkey, 0, static_cast(key_slice.size()), - reinterpret_cast(key_slice.data())); - return jkey; -} - -/* - * Class: org_rocksdb_RocksIterator - * Method: key0 - * Signature: (J)[B - */ -jbyteArray Java_org_rocksdb_RocksIterator_value0( - JNIEnv* env, jobject jobj, jlong handle) { - auto it = reinterpret_cast(handle); - rocksdb::Slice value_slice = it->value(); - - jbyteArray jkeyValue = - env->NewByteArray(static_cast(value_slice.size())); - env->SetByteArrayRegion(jkeyValue, 0, static_cast(value_slice.size()), - reinterpret_cast(value_slice.data())); - return jkeyValue; -} - -/* - * Class: org_rocksdb_RocksIterator - * Method: value0 - * Signature: (J)[B + * Method: seek0 + * Signature: (J[BI)V */ void Java_org_rocksdb_RocksIterator_seek0( JNIEnv* env, jobject jobj, jlong handle, @@ -117,8 +95,8 @@ void Java_org_rocksdb_RocksIterator_seek0( /* * Class: org_rocksdb_RocksIterator - * Method: seek0 - * Signature: (J[BI)V + * Method: status0 + * Signature: (J)V */ void Java_org_rocksdb_RocksIterator_status0( JNIEnv* env, jobject jobj, jlong handle) { @@ -134,11 +112,33 @@ void Java_org_rocksdb_RocksIterator_status0( /* * Class: org_rocksdb_RocksIterator - * Method: disposeInternal - * Signature: (J)V + * Method: key0 + * Signature: (J)[B */ -void Java_org_rocksdb_RocksIterator_disposeInternal( +jbyteArray Java_org_rocksdb_RocksIterator_key0( JNIEnv* env, jobject jobj, jlong handle) { auto it = reinterpret_cast(handle); - delete it; + rocksdb::Slice key_slice = it->key(); + + jbyteArray jkey = env->NewByteArray(static_cast(key_slice.size())); + env->SetByteArrayRegion(jkey, 0, static_cast(key_slice.size()), + reinterpret_cast(key_slice.data())); + return jkey; +} + +/* + * Class: org_rocksdb_RocksIterator + * Method: value0 + * Signature: (J)[B + */ +jbyteArray Java_org_rocksdb_RocksIterator_value0( + JNIEnv* env, jobject jobj, jlong handle) { + auto it = reinterpret_cast(handle); + rocksdb::Slice value_slice = it->value(); + + jbyteArray jkeyValue = + env->NewByteArray(static_cast(value_slice.size())); + env->SetByteArrayRegion(jkeyValue, 0, static_cast(value_slice.size()), + reinterpret_cast(value_slice.data())); + return jkeyValue; }