Fix code review comments raised in https://reviews.facebook.net/D22779

java/org/rocksdb/AbstractComparator.java

@@ -14,8 +14,22 @@ package org.rocksdb;
  *   @see org.rocksdb.Comparator
  *   @see org.rocksdb.DirectComparator
  */
-public abstract class AbstractComparator<T extends AbstractSlice> extends RocksObject {
+public abstract class AbstractComparator<T extends AbstractSlice>
+    extends RocksObject {
 
+  /**
+   * The name of the comparator.  Used to check for comparator
+   * mismatches (i.e., a DB created with one comparator is
+   * accessed using a different comparator).
+   *
+   * A new name should be used whenever
+   * the comparator implementation changes in a way that will cause
+   * the relative ordering of any two keys to change.
+   *
+   * Names starting with "rocksdb." are reserved and should not be used.
+   *
+   * @return The name of this comparator implementation
+   */
   public abstract String name();
 
   /**

java/org/rocksdb/AbstractSlice.java

@@ -13,13 +13,23 @@ package org.rocksdb;
  * should extend either of the public abstract classes:
  *   @see org.rocksdb.Slice
  *   @see org.rocksdb.DirectSlice
+ *
+ * Regards the lifecycle of Java Slices in RocksDB:
+ *   At present when you configure a Comparator from Java, it creates an
+ *   instance of a C++ BaseComparatorJniCallback subclass and
+ *   passes that to RocksDB as the comparator. That subclass of
+ *   BaseComparatorJniCallback creates the Java
+ *   {@see org.rocksdb.AbstractSlice} subclass Objects. When you dispose
+ *   the Java {@see org.rocksdb.AbstractComparator} subclass, it disposes the
+ *   C++ BaseComparatorJniCallback subclass, which in turn destroys the
+ *   Java {@see org.rocksdb.AbstractSlice} subclass Objects.
  */
 abstract class AbstractSlice<T> extends RocksObject {
 
   /**
-   * Returns the data.
+   * Returns the data of the slice.
    *
-   * @return The data. Note, the type of access is
+   * @return The slice data. Note, the type of access is
    *   determined by the subclass
    *   @see org.rocksdb.AbstractSlice#data0(long).
    */
@@ -65,7 +75,7 @@ abstract class AbstractSlice<T> extends RocksObject {
    * Creates a string representation of the data
    *
    * @param hex When true, the representation
-   *   will be encoded in hexidecimal.
+   *   will be encoded in hexadecimal.
    *
    * @return The string representation of the data.
    */
@@ -96,13 +106,13 @@ abstract class AbstractSlice<T> extends RocksObject {
   }
 
   /**
-   * If other is a slice, then
+   * If other is a slice object, then
-   * we defer to compare to check equality,
+   * we defer to {@link #compare(AbstractSlice) compare}
-   * otherwise we return false.
+   * to check equality, otherwise we return false.
    *
    * @param other Object to test for equality
    *
-   * @return true when this.compare(other) == 0,
+   * @return true when {@code this.compare(other) == 0},
    *   false otherwise.
    */
   @Override
@@ -115,13 +125,14 @@ abstract class AbstractSlice<T> extends RocksObject {
   }
 
   /**
-   * Determines whether this starts with prefix
+   * Determines whether this slice starts with
+   * another slice
    *
    * @param prefix Another slice which may of may not
-   *   be the prefix of this slice.
+   *   be a prefix of this slice.
    *
-   * @return true when slice `prefix` is a prefix
+   * @return true when this slice starts with the
-   *   of this slice
+   *   {@code prefix} slice
    */
   public boolean startsWith(final AbstractSlice prefix) {
     if (prefix != null) {

java/org/rocksdb/Comparator.java

@@ -15,7 +15,6 @@ package org.rocksdb;
  * using @see org.rocksdb.DirectComparator
  */
 public abstract class Comparator extends AbstractComparator<Slice> {
-
   public Comparator(final ComparatorOptions copt) {
     super();
     createNewComparator0(copt.nativeHandle_);

java/org/rocksdb/ComparatorOptions.java

@@ -1,7 +1,14 @@
 package org.rocksdb;
 
+/**
+ * This class controls the behaviour
+ * of Java implementations of
+ * AbstractComparator
+ *
+ * Note that dispose() must be called before a ComparatorOptions
+ * instance becomes out-of-scope to release the allocated memory in C++.
+ */
 public class ComparatorOptions extends RocksObject {
-
   public ComparatorOptions() {
     super();
     newComparatorOptions();
@@ -44,6 +51,7 @@ public class ComparatorOptions extends RocksObject {
 
   private native void newComparatorOptions();
   private native boolean useAdaptiveMutex(final long handle);
-  private native void setUseAdaptiveMutex(final long handle, final boolean useAdaptiveMutex);
+  private native void setUseAdaptiveMutex(final long handle,
+      final boolean useAdaptiveMutex);
   private native void disposeInternal(long handle);
 }

java/org/rocksdb/DirectComparator.java

@@ -15,7 +15,6 @@ package org.rocksdb;
  * using @see org.rocksdb.Comparator
  */
 public abstract class DirectComparator extends AbstractComparator<DirectSlice> {
-
   public DirectComparator(final ComparatorOptions copt) {
     super();
     createNewDirectComparator0(copt.nativeHandle_);

java/org/rocksdb/DirectSlice.java

@@ -16,11 +16,18 @@ import java.nio.ByteBuffer;
  * values consider using @see org.rocksdb.Slice
  */
 public class DirectSlice extends AbstractSlice<ByteBuffer> {
-
   /**
    * Called from JNI to construct a new Java DirectSlice
    * without an underlying C++ object set
    * at creation time.
+   *
+   * Note: You should be aware that
+   * {@see org.rocksdb.RocksObject#disOwnNativeHandle()} is intentionally
+   * called from the default DirectSlice constructor, and that it is marked as
+   * 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.
    */
   private DirectSlice() {
     super();
@@ -31,6 +38,8 @@ public class DirectSlice extends AbstractSlice<ByteBuffer> {
    * Constructs a slice
    * where the data is taken from
    * a String.
+   *
+   * @param str The string
    */
   public DirectSlice(final String str) {
     super();
@@ -41,6 +50,9 @@ public class DirectSlice extends AbstractSlice<ByteBuffer> {
    * Constructs a slice where the data is
    * read from the provided
    * ByteBuffer up to a certain length
+   *
+   * @param data The buffer containing the data
+   * @param length The length of the data to use for the slice
    */
   public DirectSlice(final ByteBuffer data, final int length) {
     super();
@@ -51,6 +63,8 @@ public class DirectSlice extends AbstractSlice<ByteBuffer> {
    * Constructs a slice where the data is
    * read from the provided
    * ByteBuffer
+   *
+   * @param data The bugger containing the data
    */
   public DirectSlice(final ByteBuffer data) {
     super();
@@ -79,7 +93,7 @@ public class DirectSlice extends AbstractSlice<ByteBuffer> {
   }
 
   /**
-   * Drops the specified n
+   * Drops the specified {@code n}
    * number of bytes from the start
    * of the backing slice
    *

java/org/rocksdb/Slice.java

@@ -14,11 +14,18 @@ package org.rocksdb;
  * values consider using @see org.rocksdb.DirectSlice
  */
 public class Slice extends AbstractSlice<byte[]> {
-
   /**
    * Called from JNI to construct a new Java Slice
    * without an underlying C++ object set
    * at creation time.
+   *
+   * Note: You should be aware that
+   * {@see org.rocksdb.RocksObject#disOwnNativeHandle()} is intentionally
+   * called from the default Slice constructor, and that it is marked as
+   * private. This is so that developers cannot construct their own default
+   * Slice objects (at present). As developers cannot construct their own
+   * Slice objects through this, they are not creating underlying C++ Slice
+   * objects, and so there is nothing to free (dispose) from Java.
    */
   private Slice() {
     super();

java/org/rocksdb/test/AbstractComparatorTest.java

@@ -52,9 +52,9 @@ public abstract class AbstractComparatorTest {
 
       db = RocksDB.open(opt, db_path.toString());
       final Random random = new Random();
-      for(int i = 0; i < ITERATIONS; i++) {
+      for (int i = 0; i < ITERATIONS; i++) {
         final byte key[] = intToByte(random.nextInt());
-        if(i > 0 && db.get(key) != null) { // does key already exist (avoid duplicates)
+        if (i > 0 && db.get(key) != null) { // does key already exist (avoid duplicates)
           i--; // generate a different key
         } else {
           db.put(key, "value".getBytes());
@@ -71,7 +71,7 @@ public abstract class AbstractComparatorTest {
       it.seekToFirst();
       int lastKey = Integer.MIN_VALUE;
       int count = 0;
-      for(it.seekToFirst(); it.isValid(); it.next()) {
+      for (it.seekToFirst(); it.isValid(); it.next()) {
         final int thisKey = byteToInt(it.key());
         assert(thisKey > lastKey);
         lastKey = thisKey;
@@ -85,11 +85,11 @@ public abstract class AbstractComparatorTest {
       System.err.format("[ERROR]: %s%n", e);
       e.printStackTrace();
     } finally {
-      if(db != null) {
+      if (db != null) {
         db.close();
       }
 
-      if(opt != null) {
+      if (opt != null) {
         opt.dispose();
       }
 
@@ -114,7 +114,7 @@ public abstract class AbstractComparatorTest {
     // protect against int key calculation overflow
     final double diff = (double)iA - iB;
     final int result;
-    if(diff < Integer.MIN_VALUE) {
+    if (diff < Integer.MIN_VALUE) {
       result = Integer.MIN_VALUE;
     } else if(diff > Integer.MAX_VALUE) {
       result = Integer.MAX_VALUE;

java/rocksjni/comparator.cc

@@ -18,27 +18,6 @@
 #include "rocksjni/comparatorjnicallback.h"
 #include "rocksjni/portal.h"
 
-// <editor-fold desc="org.rocksdb.ComparatorOptions">
-
-void Java_org_rocksdb_ComparatorOptions_newComparatorOptions(
-    JNIEnv* env, jobject jobj, jstring jpath, jboolean jshare_table_files,
-    jboolean jsync, jboolean jdestroy_old_data, jboolean jbackup_log_files,
-    jlong jbackup_rate_limit, jlong jrestore_rate_limit) {
-  jbackup_rate_limit = (jbackup_rate_limit <= 0) ? 0 : jbackup_rate_limit;
-  jrestore_rate_limit = (jrestore_rate_limit <= 0) ? 0 : jrestore_rate_limit;
-
-  const char* cpath = env->GetStringUTFChars(jpath, 0);
-
-  auto bopt = new rocksdb::BackupableDBOptions(cpath, nullptr,
-      jshare_table_files, nullptr, jsync, jdestroy_old_data, jbackup_log_files,
-      jbackup_rate_limit, jrestore_rate_limit);
-
-  env->ReleaseStringUTFChars(jpath, cpath);
-
-  rocksdb::BackupableDBOptionsJni::setHandle(env, jobj, bopt);
-}
-// </editor-fold>
-
 // <editor-fold desc="org.rocksdb.AbstractComparator>
 
 /*

java/rocksjni/comparatorjnicallback.cc

@@ -12,11 +12,9 @@
 namespace rocksdb {
 BaseComparatorJniCallback::BaseComparatorJniCallback(
     JNIEnv* env, jobject jComparator,
-    const ComparatorJniCallbackOptions* copt) {
+    const ComparatorJniCallbackOptions* copt)
-
+    : mtx_compare(new port::Mutex(copt->use_adaptive_mutex)),
-  // mutex is used for synchronisation when we are re-using
+    mtx_findShortestSeparator(new port::Mutex(copt->use_adaptive_mutex)) {
-  // the global java slice objects
-  mutex_ = new port::Mutex(copt->use_adaptive_mutex);
 
   // Note: Comparator methods may be accessed by multiple threads,
   // so we ref the jvm not the env
@@ -57,7 +55,10 @@ const char* BaseComparatorJniCallback::Name() const {
 int BaseComparatorJniCallback::Compare(const Slice& a, const Slice& b) const {
   JNIEnv* m_env = getJniEnv();
 
-  mutex_->Lock();
+  // TODO(adamretter): slice objects can potentially be cached using thread
+  // local variables to avoid locking. Could make this configurable depending on
+  // performance.
+  mtx_compare->Lock();
 
   AbstractSliceJni::setHandle(m_env, m_jSliceA, &a);
   AbstractSliceJni::setHandle(m_env, m_jSliceB, &b);
@@ -65,7 +66,7 @@ int BaseComparatorJniCallback::Compare(const Slice& a, const Slice& b) const {
     m_env->CallIntMethod(m_jComparator, m_jCompareMethodId, m_jSliceA,
       m_jSliceB);
 
-  mutex_->Unlock();
+  mtx_compare->Unlock();
 
   m_jvm->DetachCurrentThread();
 
@@ -83,14 +84,17 @@ void BaseComparatorJniCallback::FindShortestSeparator(
   const char* startUtf = start->c_str();
   jstring jsStart = m_env->NewStringUTF(startUtf);
 
-  mutex_->Lock();
+  // TODO(adamretter): slice object can potentially be cached using thread local
+  // variable to avoid locking. Could make this configurable depending on
+  // performance.
+  mtx_findShortestSeparator->Lock();
 
   AbstractSliceJni::setHandle(m_env, m_jSliceLimit, &limit);
   jstring jsResultStart =
     (jstring)m_env->CallObjectMethod(m_jComparator,
       m_jFindShortestSeparatorMethodId, jsStart, m_jSliceLimit);
 
-  mutex_->Unlock();
+  mtx_findShortestSeparator->Unlock();
 
   m_env->DeleteLocalRef(jsStart);
 
@@ -120,9 +124,8 @@ void BaseComparatorJniCallback::FindShortSuccessor(std::string* key) const {
   m_env->DeleteLocalRef(jsKey);
 
   if (jsResultKey != nullptr) {
-    // update key with result
+    // updates key with result, also releases jsResultKey.
-    *key =
+    *key = JniUtil::copyString(m_env, jsResultKey);
-      JniUtil::copyString(m_env, jsResultKey);  // also releases jsResultKey
   }
 
   m_jvm->DetachCurrentThread();
@@ -132,9 +135,6 @@ BaseComparatorJniCallback::~BaseComparatorJniCallback() {
   JNIEnv* m_env = getJniEnv();
 
   m_env->DeleteGlobalRef(m_jComparator);
-  m_env->DeleteGlobalRef(m_jSliceA);
-  m_env->DeleteGlobalRef(m_jSliceB);
-  m_env->DeleteGlobalRef(m_jSliceLimit);
 
   // Note: do not need to explicitly detach, as this function is effectively
   // called from the Java class's disposeInternal method, and so already
@@ -151,6 +151,13 @@ ComparatorJniCallback::ComparatorJniCallback(
   m_jSliceLimit = env->NewGlobalRef(SliceJni::construct0(env));
 }
 
+ComparatorJniCallback::~ComparatorJniCallback() {
+  JNIEnv* m_env = getJniEnv();
+  m_env->DeleteGlobalRef(m_jSliceA);
+  m_env->DeleteGlobalRef(m_jSliceB);
+  m_env->DeleteGlobalRef(m_jSliceLimit);
+}
+
 DirectComparatorJniCallback::DirectComparatorJniCallback(
     JNIEnv* env, jobject jComparator,
     const ComparatorJniCallbackOptions* copt) :
@@ -159,4 +166,11 @@ DirectComparatorJniCallback::DirectComparatorJniCallback(
   m_jSliceB = env->NewGlobalRef(DirectSliceJni::construct0(env));
   m_jSliceLimit = env->NewGlobalRef(DirectSliceJni::construct0(env));
 }
+
+DirectComparatorJniCallback::~DirectComparatorJniCallback() {
+  JNIEnv* m_env = getJniEnv();
+  m_env->DeleteGlobalRef(m_jSliceA);
+  m_env->DeleteGlobalRef(m_jSliceB);
+  m_env->DeleteGlobalRef(m_jSliceLimit);
+}
 }  // namespace rocksdb

java/rocksjni/comparatorjnicallback.h

@@ -41,7 +41,8 @@ struct ComparatorJniCallbackOptions {
  * method callbacks. Instead of creating new objects for each callback
  * of those functions, by reuse via setHandle we are a lot
  * faster; Unfortunately this means that we have to
- * introduce locking in regions of those methods via mutex_.
+ * introduce independent locking in regions of each of those methods
+ * via the mutexs mtx_compare and mtx_findShortestSeparator respectively
  */
 class BaseComparatorJniCallback : public Comparator {
  public:
@@ -56,16 +57,19 @@ class BaseComparatorJniCallback : public Comparator {
     virtual void FindShortSuccessor(std::string* key) const;
 
  private:
-    port::Mutex* mutex_;
+    // used for synchronisation in compare method
+    port::Mutex* mtx_compare;
+    // used for synchronisation in findShortestSeparator method
+    port::Mutex* mtx_findShortestSeparator;
     JavaVM* m_jvm;
     jobject m_jComparator;
     std::string m_name;
     jmethodID m_jCompareMethodId;
     jmethodID m_jFindShortestSeparatorMethodId;
     jmethodID m_jFindShortSuccessorMethodId;
-    JNIEnv* getJniEnv() const;
 
  protected:
+    JNIEnv* getJniEnv() const;
     jobject m_jSliceA;
     jobject m_jSliceB;
     jobject m_jSliceLimit;
@@ -76,6 +80,7 @@ class ComparatorJniCallback : public BaseComparatorJniCallback {
       ComparatorJniCallback(
         JNIEnv* env, jobject jComparator,
         const ComparatorJniCallbackOptions* copt);
+      ~ComparatorJniCallback();
 };
 
 class DirectComparatorJniCallback : public BaseComparatorJniCallback {
@@ -83,6 +88,7 @@ class DirectComparatorJniCallback : public BaseComparatorJniCallback {
       DirectComparatorJniCallback(
         JNIEnv* env, jobject jComparator,
         const ComparatorJniCallbackOptions* copt);
+      ~DirectComparatorJniCallback();
 };
 }  // namespace rocksdb