Remove allow_thread_local

Summary: See https://reviews.facebook.net/D19365

Test Plan: compiles

Reviewers: sdong, yhchiang, ljin

Reviewed By: ljin

Subscribers: leveldb

Differential Revision: https://reviews.facebook.net/D23907

Author: igorcanadi

HISTORY.md

@@ -8,7 +8,10 @@
 * We have refactored our system of stalling writes.  Any stall-related statistics' meanings are changed. Instead of per-write stall counts, we now count stalls per-epoch, where epochs are periods between flushes and compactions. You'll find more information in our Tuning Perf Guide once we release RocksDB 3.6.
 * When disableDataSync=true, we no longer sync the MANIFEST file.
 * Add identity_as_first_hash property to CuckooTable. SST file needs to be rebuilt to be opened by reader properly.
+
+### Public API changes
 * Change target_file_size_base type to uint64_t from int.
+* Remove allow_thread_local. This feature was proved to be stable, so we are turning it always-on.
 
 ----- Past Releases -----
 

db/c.cc

@@ -1355,8 +1355,8 @@ void rocksdb_options_set_purge_redundant_kvs_while_flush(
   opt->rep.purge_redundant_kvs_while_flush = v;
 }
 
-void rocksdb_options_set_allow_os_buffer(
-    rocksdb_options_t* opt, unsigned char v) {
+void rocksdb_options_set_allow_os_buffer(rocksdb_options_t* opt,
+                                         unsigned char v) {
   opt->rep.allow_os_buffer = v;
 }
 
@@ -1581,11 +1581,6 @@ void rocksdb_options_set_bloom_locality(
   opt->rep.bloom_locality = v;
 }
 
-void rocksdb_options_set_allow_thread_local(
-    rocksdb_options_t* opt, unsigned char v) {
-  opt->rep.allow_thread_local = v;
-}
-
 void rocksdb_options_set_inplace_update_support(
     rocksdb_options_t* opt, unsigned char v) {
   opt->rep.inplace_update_support = v;

db/column_family.cc

@@ -411,16 +411,10 @@ Compaction* ColumnFamilyData::CompactRange(int input_level, int output_level,
 SuperVersion* ColumnFamilyData::GetReferencedSuperVersion(
     port::Mutex* db_mutex) {
   SuperVersion* sv = nullptr;
-  if (LIKELY(column_family_set_->db_options_->allow_thread_local)) {
-    sv = GetThreadLocalSuperVersion(db_mutex);
-    sv->Ref();
-    if (!ReturnThreadLocalSuperVersion(sv)) {
-      sv->Unref();
-    }
-  } else {
-    db_mutex->Lock();
-    sv = super_version_->Ref();
-    db_mutex->Unlock();
+  sv = GetThreadLocalSuperVersion(db_mutex);
+  sv->Ref();
+  if (!ReturnThreadLocalSuperVersion(sv)) {
+    sv->Unref();
   }
   return sv;
 }
@@ -506,9 +500,7 @@ SuperVersion* ColumnFamilyData::InstallSuperVersion(
   ++super_version_number_;
   super_version_->version_number = super_version_number_;
   // Reset SuperVersions cached in thread local storage
-  if (column_family_set_->db_options_->allow_thread_local) {
-    ResetThreadLocalSuperVersions();
-  }
+  ResetThreadLocalSuperVersions();
 
   RecalculateWriteStallConditions();
 

db/db_impl.cc

@@ -401,24 +401,22 @@ DBImpl::~DBImpl() {
     mutex_.Lock();
   }
 
-  if (db_options_.allow_thread_local) {
-    // Clean up obsolete files due to SuperVersion release.
-    // (1) Need to delete to obsolete files before closing because RepairDB()
-    // scans all existing files in the file system and builds manifest file.
-    // Keeping obsolete files confuses the repair process.
-    // (2) Need to check if we Open()/Recover() the DB successfully before
-    // deleting because if VersionSet recover fails (may be due to corrupted
-    // manifest file), it is not able to identify live files correctly. As a
-    // result, all "live" files can get deleted by accident. However, corrupted
-    // manifest is recoverable by RepairDB().
-    if (opened_successfully_) {
-      DeletionState deletion_state;
-      FindObsoleteFiles(deletion_state, true);
-      // manifest number starting from 2
-      deletion_state.manifest_file_number = 1;
-      if (deletion_state.HaveSomethingToDelete()) {
-        PurgeObsoleteFiles(deletion_state);
-      }
+  // Clean up obsolete files due to SuperVersion release.
+  // (1) Need to delete to obsolete files before closing because RepairDB()
+  // scans all existing files in the file system and builds manifest file.
+  // Keeping obsolete files confuses the repair process.
+  // (2) Need to check if we Open()/Recover() the DB successfully before
+  // deleting because if VersionSet recover fails (may be due to corrupted
+  // manifest file), it is not able to identify live files correctly. As a
+  // result, all "live" files can get deleted by accident. However, corrupted
+  // manifest is recoverable by RepairDB().
+  if (opened_successfully_) {
+    DeletionState deletion_state;
+    FindObsoleteFiles(deletion_state, true);
+    // manifest number starting from 2
+    deletion_state.manifest_file_number = 1;
+    if (deletion_state.HaveSomethingToDelete()) {
+      PurgeObsoleteFiles(deletion_state);
     }
   }
 
@@ -4315,20 +4313,12 @@ bool DBImpl::GetIntPropertyInternal(ColumnFamilyHandle* column_family,
 
 SuperVersion* DBImpl::GetAndRefSuperVersion(ColumnFamilyData* cfd) {
   // TODO(ljin): consider using GetReferencedSuperVersion() directly
-  if (LIKELY(db_options_.allow_thread_local)) {
-    return cfd->GetThreadLocalSuperVersion(&mutex_);
-  } else {
-    MutexLock l(&mutex_);
-    return cfd->GetSuperVersion()->Ref();
-  }
+  return cfd->GetThreadLocalSuperVersion(&mutex_);
 }
 
 void DBImpl::ReturnAndCleanupSuperVersion(ColumnFamilyData* cfd,
                                           SuperVersion* sv) {
-  bool unref_sv = true;
-  if (LIKELY(db_options_.allow_thread_local)) {
-    unref_sv = !cfd->ReturnThreadLocalSuperVersion(sv);
-  }
+  bool unref_sv = !cfd->ReturnThreadLocalSuperVersion(sv);
 
   if (unref_sv) {
     // Release SuperVersion

include/rocksdb/c.h

@@ -537,8 +537,6 @@ extern void rocksdb_options_set_min_partial_merge_operands(
     rocksdb_options_t*, uint32_t);
 extern void rocksdb_options_set_bloom_locality(
     rocksdb_options_t*, uint32_t);
-extern void rocksdb_options_set_allow_thread_local(
-    rocksdb_options_t*, unsigned char);
 extern void rocksdb_options_set_inplace_update_support(
     rocksdb_options_t*, unsigned char);
 extern void rocksdb_options_set_inplace_update_num_locks(

include/rocksdb/options.h

@@ -799,10 +799,6 @@ struct DBOptions {
   // Default: false
   bool use_adaptive_mutex;
 
-  // Allow RocksDB to use thread local storage to optimize performance.
-  // Default: true
-  bool allow_thread_local;
-
   // Create DBOptions with default values for all fields
   DBOptions();
   // Create DBOptions from Options

java/org/rocksdb/Options.java

@@ -1070,33 +1070,6 @@ public Options setBytesPerSync(long bytesPerSync) {
   private native void setBytesPerSync(
       long handle, long bytesPerSync);
 
-  /**
-   * Allow RocksDB to use thread local storage to optimize performance.
-   * Default: true
-   *
-   * @return true if thread-local storage is allowed
-   */
-  public boolean allowThreadLocal() {
-    assert(isInitialized());
-    return allowThreadLocal(nativeHandle_);
-  }
-  private native boolean allowThreadLocal(long handle);
-
-  /**
-   * Allow RocksDB to use thread local storage to optimize performance.
-   * Default: true
-   *
-   * @param allowThreadLocal true if thread-local storage is allowed.
-   * @return the reference to the current option.
-   */
-  public Options setAllowThreadLocal(boolean allowThreadLocal) {
-    assert(isInitialized());
-    setAllowThreadLocal(nativeHandle_, allowThreadLocal);
-    return this;
-  }
-  private native void setAllowThreadLocal(
-      long handle, boolean allowThreadLocal);
-
   /**
    * Set the config for mem-table.
    *

java/org/rocksdb/test/OptionsTest.java

@@ -184,12 +184,6 @@ public static void main(String[] args) {
       assert(opt.bytesPerSync() == longValue);
     }
 
-    { // AllowThreadLocal test
-      boolean boolValue = rand.nextBoolean();
-      opt.setAllowThreadLocal(boolValue);
-      assert(opt.allowThreadLocal() == boolValue);
-    }
-
     { // WriteBufferSize test
       long longValue = rand.nextLong();
       opt.setWriteBufferSize(longValue);

java/rocksjni/options.cc

@@ -789,27 +789,6 @@ void Java_org_rocksdb_Options_setBytesPerSync(
       static_cast<int64_t>(bytes_per_sync);
 }
 
-/*
- * Class:     org_rocksdb_Options
- * Method:    allowThreadLocal
- * Signature: (J)Z
- */
-jboolean Java_org_rocksdb_Options_allowThreadLocal(
-    JNIEnv* env, jobject jobj, jlong jhandle) {
-  return reinterpret_cast<rocksdb::Options*>(jhandle)->allow_thread_local;
-}
-
-/*
- * Class:     org_rocksdb_Options
- * Method:    setAllowThreadLocal
- * Signature: (JZ)V
- */
-void Java_org_rocksdb_Options_setAllowThreadLocal(
-    JNIEnv* env, jobject jobj, jlong jhandle, jboolean allow_thread_local) {
-  reinterpret_cast<rocksdb::Options*>(jhandle)->allow_thread_local =
-      static_cast<bool>(allow_thread_local);
-}
-
 /*
  * Method:    tableFactoryName
  * Signature: (J)Ljava/lang/String

util/options.cc

@@ -213,7 +213,6 @@ DBOptions::DBOptions()
       advise_random_on_open(true),
       access_hint_on_compaction_start(NORMAL),
       use_adaptive_mutex(false),
-      allow_thread_local(true),
       bytes_per_sync(0) {}
 
 DBOptions::DBOptions(const Options& options)
@@ -256,7 +255,6 @@ DBOptions::DBOptions(const Options& options)
       advise_random_on_open(options.advise_random_on_open),
       access_hint_on_compaction_start(options.access_hint_on_compaction_start),
       use_adaptive_mutex(options.use_adaptive_mutex),
-      allow_thread_local(options.allow_thread_local),
       bytes_per_sync(options.bytes_per_sync) {}
 
 static const char* const access_hints[] = {

util/options_helper.cc

@@ -301,8 +301,6 @@ bool GetOptionsFromStrings(
         new_options->advise_random_on_open = ParseBoolean(o.first, o.second);
       } else if (o.first == "use_adaptive_mutex") {
         new_options->use_adaptive_mutex = ParseBoolean(o.first, o.second);
-      } else if (o.first == "allow_thread_local") {
-        new_options->allow_thread_local = ParseBoolean(o.first, o.second);
       } else if (o.first == "bytes_per_sync") {
         new_options->bytes_per_sync = ParseUint64(o.second);
       } else {

util/options_test.cc

@@ -151,7 +151,6 @@ TEST(OptionsTest, GetOptionsFromStringsTest) {
     {"stats_dump_period_sec", "46"},
     {"advise_random_on_open", "true"},
     {"use_adaptive_mutex", "false"},
-    {"allow_thread_local", "true"},
     {"bytes_per_sync", "47"},
   };
 
@@ -239,7 +238,6 @@ TEST(OptionsTest, GetOptionsFromStringsTest) {
   ASSERT_EQ(new_opt.stats_dump_period_sec, 46U);
   ASSERT_EQ(new_opt.advise_random_on_open, true);
   ASSERT_EQ(new_opt.use_adaptive_mutex, false);
-  ASSERT_EQ(new_opt.allow_thread_local, true);
   ASSERT_EQ(new_opt.bytes_per_sync, static_cast<uint64_t>(47));
 
   options_map["write_buffer_size"] = "hello";