Clean up some CFOptions code hygiene, unnecessary copies

Summary: To start, I wanted to remove the unnecessary new_options parameter of `InstallSuperVersionAndScheduleWork()`. Passing it something other than the current mutable options would be inconsistent/outdated. There was even a comment "Use latest MutableCFOptions" on a place that was using the saved options in effect for the compaction. And remove some unnecessary copying of entire MutableCFOptions while holding DB mutex, as in `DBImpl::ReFitLevel()`, `DBImplFollower::TryCatchUpWithLeader()`, `DBImplSecondary::RecoverLogFiles()`. (`ConstructNewMemtable()` doesn't save a reference.) Intended follow-up: Clarify/simplify the crazy calling conventions of LogAndApply (see new FIXME) Test Plan: existing tests and CI
pdillinger · Jan 14, 2025 · 4aa46ac · 4aa46ac
1 parent 6e97a81
commit 4aa46ac
Show file tree

Hide file tree

Showing 44 changed files with 446 additions and 482 deletions.
diff --git a/db/arena_wrapped_db_iter.cc b/db/arena_wrapped_db_iter.cc
@@ -97,7 +97,7 @@ Status ArenaWrappedDBIter::Refresh(const Snapshot* snapshot) {
     if (read_callback_) {
       read_callback_->Refresh(read_seq);
     }
-    Init(env, read_options_, *(cfd->ioptions()), sv->mutable_cf_options,
+    Init(env, read_options_, cfd->ioptions(), sv->mutable_cf_options,
          sv->current, read_seq,
          sv->mutable_cf_options.max_sequential_skip_in_iterations,
          sv->version_number, read_callback_, cfh_, expose_blob_index_,

diff --git a/db/column_family.cc b/db/column_family.cc
@@ -53,7 +53,7 @@ ColumnFamilyHandleImpl::ColumnFamilyHandleImpl(
 
 ColumnFamilyHandleImpl::~ColumnFamilyHandleImpl() {
   if (cfd_ != nullptr) {
-    for (auto& listener : cfd_->ioptions()->listeners) {
+    for (auto& listener : cfd_->ioptions().listeners) {
       listener->OnColumnFamilyHandleDeletionStarted(this);
     }
     // Job id == 0 means that this is not our background process, but rather
@@ -593,7 +593,7 @@ ColumnFamilyData::ColumnFamilyData(
                                       block_cache_tracer, io_tracer,
                                       db_session_id));
     blob_file_cache_.reset(
-        new BlobFileCache(_table_cache, ioptions(), soptions(), id_,
+        new BlobFileCache(_table_cache, &ioptions(), soptions(), id_,
                           internal_stats_->GetBlobFileReadHist(), io_tracer));
     blob_source_.reset(new BlobSource(ioptions_, mutable_cf_options_, db_id,
                                       db_session_id, blob_file_cache_.get()));
@@ -968,7 +968,7 @@ WriteStallCondition ColumnFamilyData::RecalculateWriteStallConditions(
     auto write_stall_condition_and_cause = GetWriteStallConditionAndCause(
         imm()->NumNotFlushed(), vstorage->l0_delay_trigger_count(),
         vstorage->estimated_compaction_needed_bytes(), mutable_cf_options,
-        *ioptions());
+        ioptions());
     write_stall_condition = write_stall_condition_and_cause.first;
     auto write_stall_cause = write_stall_condition_and_cause.second;
 
@@ -1384,7 +1384,7 @@ void ColumnFamilyData::InstallSuperVersion(
         new_superversion->write_stall_condition) {
       sv_context->PushWriteStallNotification(
           old_superversion->write_stall_condition,
-          new_superversion->write_stall_condition, GetName(), ioptions());
+          new_superversion->write_stall_condition, GetName(), &ioptions());
     }
     if (old_superversion->Unref()) {
       old_superversion->Cleanup();
@@ -1844,10 +1844,7 @@ const ImmutableOptions& GetImmutableOptions(ColumnFamilyHandle* column_family) {
   const ColumnFamilyData* const cfd = handle->cfd();
   assert(cfd);
 
-  const ImmutableOptions* ioptions = cfd->ioptions();
-  assert(ioptions);
-
-  return *ioptions;
+  return cfd->ioptions();
 }
 
 }  // namespace ROCKSDB_NAMESPACE
diff --git a/db/column_family.h b/db/column_family.h
@@ -221,7 +221,8 @@ struct SuperVersion {
   // enable UDT feature, this is an empty string.
   std::string full_history_ts_low;
 
-  // A shared copy of the DB's seqno to time mapping.
+  // An immutable snapshot of the DB's seqno to time mapping, usually shared
+  // between SuperVersions.
   std::shared_ptr<const SeqnoToTimeMapping> seqno_to_time_mapping{nullptr};
 
   // should be called outside the mutex
@@ -342,17 +343,17 @@ class ColumnFamilyData {
 
   // thread-safe
   const FileOptions* soptions() const;
-  const ImmutableOptions* ioptions() const { return &ioptions_; }
+  const ImmutableOptions& ioptions() const { return ioptions_; }
   // REQUIRES: DB mutex held
   // This returns the MutableCFOptions used by current SuperVersion
   // You should use this API to reference MutableCFOptions most of the time.
-  const MutableCFOptions* GetCurrentMutableCFOptions() const {
-    return &(super_version_->mutable_cf_options);
+  const MutableCFOptions& GetCurrentMutableCFOptions() const {
+    return super_version_->mutable_cf_options;
   }
   // REQUIRES: DB mutex held
   // This returns the latest MutableCFOptions, which may be not in effect yet.
-  const MutableCFOptions* GetLatestMutableCFOptions() const {
-    return &mutable_cf_options_;
+  const MutableCFOptions& GetLatestMutableCFOptions() const {
+    return mutable_cf_options_;
   }
 
   // REQUIRES: DB mutex held

diff --git a/db/compaction/compaction.cc b/db/compaction/compaction.cc
@@ -332,11 +332,11 @@ Compaction::Compaction(
               : (_blob_garbage_collection_policy ==
                          BlobGarbageCollectionPolicy::kDisable
                      ? false
-                     : mutable_cf_options()->enable_blob_garbage_collection)),
+                     : mutable_cf_options().enable_blob_garbage_collection)),
       blob_garbage_collection_age_cutoff_(
           _blob_garbage_collection_age_cutoff < 0 ||
                   _blob_garbage_collection_age_cutoff > 1
-              ? mutable_cf_options()->blob_garbage_collection_age_cutoff
+              ? mutable_cf_options().blob_garbage_collection_age_cutoff
               : _blob_garbage_collection_age_cutoff),
       penultimate_level_(
           // For simplicity, we don't support the concept of "penultimate level"
@@ -592,7 +592,7 @@ bool Compaction::IsTrivialMove() const {
   // input files are non overlapping
   if ((mutable_cf_options_.compaction_options_universal.allow_trivial_move) &&
       (output_level_ != 0) &&
-      (cfd_->ioptions()->compaction_style == kCompactionStyleUniversal)) {
+      (cfd_->ioptions().compaction_style == kCompactionStyleUniversal)) {
     return is_trivial_move_;
   }
 
@@ -650,7 +650,7 @@ bool Compaction::KeyNotExistsBeyondOutputLevel(
   if (bottommost_level_) {
     return true;
   } else if (output_level_ != 0 &&
-             cfd_->ioptions()->compaction_style == kCompactionStyleLevel) {
+             cfd_->ioptions().compaction_style == kCompactionStyleLevel) {
     // Maybe use binary search to find right entry instead of linear search?
     const Comparator* user_cmp = cfd_->user_comparator();
     for (int lvl = output_level_ + 1; lvl < number_levels_; lvl++) {
@@ -691,7 +691,7 @@ bool Compaction::KeyRangeNotExistsBeyondOutputLevel(
   if (bottommost_level_) {
     return true /* does not overlap */;
   } else if (output_level_ != 0 &&
-             cfd_->ioptions()->compaction_style == kCompactionStyleLevel) {
+             cfd_->ioptions().compaction_style == kCompactionStyleLevel) {
     const Comparator* user_cmp = cfd_->user_comparator();
     for (int lvl = output_level_ + 1; lvl < number_levels_; lvl++) {
       const std::vector<FileMetaData*>& files =
@@ -867,12 +867,12 @@ uint64_t Compaction::OutputFilePreallocationSize() const {
 }
 
 std::unique_ptr<CompactionFilter> Compaction::CreateCompactionFilter() const {
-  if (!cfd_->ioptions()->compaction_filter_factory) {
+  if (!cfd_->ioptions().compaction_filter_factory) {
     return nullptr;
   }
 
   if (!cfd_->ioptions()
-           ->compaction_filter_factory->ShouldFilterTableFileCreation(
+           .compaction_filter_factory->ShouldFilterTableFileCreation(
                TableFileCreationReason::kCompaction)) {
     return nullptr;
   }
@@ -891,7 +891,7 @@ std::unique_ptr<CompactionFilter> Compaction::CreateCompactionFilter() const {
         "for compaction.");
   }
 
-  return cfd_->ioptions()->compaction_filter_factory->CreateCompactionFilter(
+  return cfd_->ioptions().compaction_filter_factory->CreateCompactionFilter(
       context);
 }
 
@@ -925,18 +925,18 @@ bool Compaction::ShouldFormSubcompactions() const {
 
   // Round-Robin pri under leveled compaction allows subcompactions by default
   // and the number of subcompactions can be larger than max_subcompactions_
-  if (cfd_->ioptions()->compaction_pri == kRoundRobin &&
-      cfd_->ioptions()->compaction_style == kCompactionStyleLevel) {
+  if (cfd_->ioptions().compaction_pri == kRoundRobin &&
+      cfd_->ioptions().compaction_style == kCompactionStyleLevel) {
     return output_level_ > 0;
   }
 
   if (max_subcompactions_ <= 1) {
     return false;
   }
 
-  if (cfd_->ioptions()->compaction_style == kCompactionStyleLevel) {
+  if (cfd_->ioptions().compaction_style == kCompactionStyleLevel) {
     return (start_level_ == 0 || is_manual_compaction_) && output_level_ > 0;
-  } else if (cfd_->ioptions()->compaction_style == kCompactionStyleUniversal) {
+  } else if (cfd_->ioptions().compaction_style == kCompactionStyleUniversal) {
     return number_levels_ > 1 && output_level_ > 0;
   } else {
     return false;

diff --git a/db/compaction/compaction.h b/db/compaction/compaction.h
@@ -288,14 +288,14 @@ class Compaction {
 
   // Return the ImmutableOptions that should be used throughout the compaction
   // procedure
-  const ImmutableOptions* immutable_options() const {
-    return &immutable_options_;
+  const ImmutableOptions& immutable_options() const {
+    return immutable_options_;
   }
 
   // Return the MutableCFOptions that should be used throughout the compaction
   // procedure
-  const MutableCFOptions* mutable_cf_options() const {
-    return &mutable_cf_options_;
+  const MutableCFOptions& mutable_cf_options() const {
+    return mutable_cf_options_;
   }
 
   // Returns the size in bytes that the output file should be preallocated to.

diff --git a/db/compaction/compaction_iterator.h b/db/compaction/compaction_iterator.h
@@ -124,8 +124,6 @@ class CompactionIterator {
     explicit RealCompaction(const Compaction* compaction)
         : compaction_(compaction) {
       assert(compaction_);
-      assert(compaction_->immutable_options());
-      assert(compaction_->mutable_cf_options());
     }
 
     int level() const override { return compaction_->level(); }
@@ -147,11 +145,11 @@ class CompactionIterator {
     }
 
     bool allow_ingest_behind() const override {
-      return compaction_->immutable_options()->allow_ingest_behind;
+      return compaction_->immutable_options().allow_ingest_behind;
     }
 
     bool allow_mmap_reads() const override {
-      return compaction_->immutable_options()->allow_mmap_reads;
+      return compaction_->immutable_options().allow_mmap_reads;
     }
 
     bool enable_blob_garbage_collection() const override {
@@ -163,7 +161,7 @@ class CompactionIterator {
     }
 
     uint64_t blob_compaction_readahead_size() const override {
-      return compaction_->mutable_cf_options()->blob_compaction_readahead_size;
+      return compaction_->mutable_cf_options().blob_compaction_readahead_size;
     }
 
     const Version* input_version() const override {