From 42eaa45c1bfcc33935d3cfdc4694955e2f1f3265 Mon Sep 17 00:00:00 2001 From: Jay Zhuang Date: Wed, 21 Jul 2021 13:44:39 -0700 Subject: [PATCH] Avoid updating option if there's no value updated (#8518) Summary: Try avoid expensive updating options operation if `SetDBOptions()` does not change any option value. Skip updating is not guaranteed, for example, changing `bytes_per_sync` to `0` may still trigger updating, as the value could be sanitized. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8518 Test Plan: added unittest Reviewed By: riversand963 Differential Revision: D29672639 Pulled By: jay-zhuang fbshipit-source-id: b7931de62ceea6f1bdff0d1209adf1197d3ed1f4 --- HISTORY.md | 4 +++ db/db_impl/db_impl.cc | 12 +++++++++ db/db_options_test.cc | 60 +++++++++++++++++++++++++++++++++++++++++++ options/db_options.cc | 9 +++++++ options/db_options.h | 3 +++ 5 files changed, 88 insertions(+) diff --git a/HISTORY.md b/HISTORY.md index 626dce1666..003327cfd5 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -21,6 +21,10 @@ ### Public API change * Added APIs to the Customizable class to allow developers to create their own Customizable classes. Created the utilities/customizable_util.h file to contain helper methods for developing new Customizable classes. * Change signature of SecondaryCache::Name(). Make SecondaryCache customizable and add SecondaryCache::CreateFromString method. + +### Performance Improvements +* Try to avoid updating DBOptions if `SetDBOptions()` does not change any option value. + ## 6.22.0 (2021-06-18) ### Behavior Changes * Added two additional tickers, MEMTABLE_PAYLOAD_BYTES_AT_FLUSH and MEMTABLE_GARBAGE_BYTES_AT_FLUSH. These stats can be used to estimate the ratio of "garbage" (outdated) bytes in the memtable that are discarded at flush time. diff --git a/db/db_impl/db_impl.cc b/db/db_impl/db_impl.cc index 20ebbfed23..95eedbf49d 100644 --- a/db/db_impl/db_impl.cc +++ b/db/db_impl/db_impl.cc @@ -1136,9 +1136,19 @@ Status DBImpl::SetDBOptions( InstrumentedMutexLock l(&mutex_); s = GetMutableDBOptionsFromStrings(mutable_db_options_, options_map, &new_options); + if (new_options.bytes_per_sync == 0) { new_options.bytes_per_sync = 1024 * 1024; } + + if (MutableDBOptionsAreEqual(mutable_db_options_, new_options)) { + ROCKS_LOG_INFO(immutable_db_options_.info_log, + "SetDBOptions(), input option value is not changed, " + "skipping updating."); + persist_options_status.PermitUncheckedError(); + return s; + } + DBOptions new_db_options = BuildDBOptions(immutable_db_options_, new_options); if (s.ok()) { @@ -4194,6 +4204,8 @@ Status DBImpl::WriteOptionsFile(bool need_mutex_lock, TEST_SYNC_POINT("DBImpl::WriteOptionsFile:1"); TEST_SYNC_POINT("DBImpl::WriteOptionsFile:2"); + TEST_SYNC_POINT_CALLBACK("DBImpl::WriteOptionsFile:PersistOptions", + &db_options); std::string file_name = TempOptionsFileName(GetName(), versions_->NewFileNumber()); diff --git a/db/db_options_test.cc b/db/db_options_test.cc index 96fd37357d..08a274c435 100644 --- a/db/db_options_test.cc +++ b/db/db_options_test.cc @@ -98,6 +98,66 @@ TEST_F(DBOptionsTest, ImmutableTrackAndVerifyWalsInManifest) { // RocksDB lite don't support dynamic options. #ifndef ROCKSDB_LITE +TEST_F(DBOptionsTest, AvoidUpdatingOptions) { + Options options; + options.env = env_; + options.max_background_jobs = 4; + options.delayed_write_rate = 1024; + + Reopen(options); + + SyncPoint::GetInstance()->DisableProcessing(); + SyncPoint::GetInstance()->ClearAllCallBacks(); + bool is_changed_stats = false; + SyncPoint::GetInstance()->SetCallBack( + "DBImpl::WriteOptionsFile:PersistOptions", [&](void* /*arg*/) { + ASSERT_FALSE(is_changed_stats); // should only save options file once + is_changed_stats = true; + }); + SyncPoint::GetInstance()->EnableProcessing(); + + // helper function to check the status and reset after each check + auto is_changed = [&] { + bool ret = is_changed_stats; + is_changed_stats = false; + return ret; + }; + + // without changing the value, but it's sanitized to a different value + ASSERT_OK(dbfull()->SetDBOptions({{"bytes_per_sync", "0"}})); + ASSERT_TRUE(is_changed()); + + // without changing the value + ASSERT_OK(dbfull()->SetDBOptions({{"max_background_jobs", "4"}})); + ASSERT_FALSE(is_changed()); + + // changing the value + ASSERT_OK(dbfull()->SetDBOptions({{"bytes_per_sync", "123"}})); + ASSERT_TRUE(is_changed()); + + // update again + ASSERT_OK(dbfull()->SetDBOptions({{"bytes_per_sync", "123"}})); + ASSERT_FALSE(is_changed()); + + // without changing a default value + ASSERT_OK(dbfull()->SetDBOptions({{"strict_bytes_per_sync", "false"}})); + ASSERT_FALSE(is_changed()); + + // now change + ASSERT_OK(dbfull()->SetDBOptions({{"strict_bytes_per_sync", "true"}})); + ASSERT_TRUE(is_changed()); + + // multiple values without change + ASSERT_OK(dbfull()->SetDBOptions( + {{"max_total_wal_size", "0"}, {"stats_dump_period_sec", "600"}})); + ASSERT_FALSE(is_changed()); + + // multiple values with change + ASSERT_OK(dbfull()->SetDBOptions( + {{"max_open_files", "100"}, {"stats_dump_period_sec", "600"}})); + ASSERT_TRUE(is_changed()); +} + TEST_F(DBOptionsTest, GetLatestDBOptions) { // GetOptions should be able to get latest option changed by SetOptions. Options options; diff --git a/options/db_options.cc b/options/db_options.cc index 17f96a553d..53533c2524 100644 --- a/options/db_options.cc +++ b/options/db_options.cc @@ -872,6 +872,15 @@ Status GetMutableDBOptionsFromStrings( return s; } +bool MutableDBOptionsAreEqual(const MutableDBOptions& this_options, + const MutableDBOptions& that_options) { + ConfigOptions config_options; + std::string mismatch; + return OptionTypeInfo::StructsAreEqual( + config_options, "MutableDBOptions", &db_mutable_options_type_info, + "MutableDBOptions", &this_options, &that_options, &mismatch); +} + Status GetStringFromMutableDBOptions(const ConfigOptions& config_options, const MutableDBOptions& mutable_opts, std::string* opt_string) { diff --git a/options/db_options.h b/options/db_options.h index 7ff90318c8..e609563d21 100644 --- a/options/db_options.h +++ b/options/db_options.h @@ -141,6 +141,9 @@ Status GetMutableDBOptionsFromStrings( const MutableDBOptions& base_options, const std::unordered_map& options_map, MutableDBOptions* new_options); + +bool MutableDBOptionsAreEqual(const MutableDBOptions& this_options, + const MutableDBOptions& that_options); #endif // ROCKSDB_LITE } // namespace ROCKSDB_NAMESPACE