From 983fafa56cccf84aa9ee7e0e1117cf71a2243709 Mon Sep 17 00:00:00 2001 From: Igor Canadi Date: Sat, 25 Jan 2014 13:50:30 -0800 Subject: [PATCH 1/3] Fix memory leak --- db/memtablelist.cc | 2 ++ db/memtablelist.h | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/db/memtablelist.cc b/db/memtablelist.cc index b52563ae8..884e326aa 100644 --- a/db/memtablelist.cc +++ b/db/memtablelist.cc @@ -31,6 +31,7 @@ MemTableListVersion::MemTableListVersion(MemTableListVersion* old) { void MemTableListVersion::Ref() { ++refs_; } void MemTableListVersion::Unref(std::vector* to_delete) { + assert(refs_ >= 1); --refs_; if (refs_ == 0) { // if to_delete is equal to nullptr it means we're confident @@ -255,6 +256,7 @@ void MemTableList::InstallNewVersion() { // somebody else holds the current version, we need to create new one MemTableListVersion* version = current_; current_ = new MemTableListVersion(current_); + current_->Ref(); version->Unref(); } } diff --git a/db/memtablelist.h b/db/memtablelist.h index 354e9872a..16bb49743 100644 --- a/db/memtablelist.h +++ b/db/memtablelist.h @@ -49,7 +49,7 @@ class MemTableListVersion { friend class MemTableList; std::list memlist_; int size_ = 0; - int refs_ = 1; + int refs_ = 0; }; // This class stores references to all the immutable memtables. From e55b3c040cc17a17e42afc5c5204394ff96babf5 Mon Sep 17 00:00:00 2001 From: Igor Canadi Date: Sun, 26 Jan 2014 17:40:43 -0800 Subject: [PATCH 2/3] Fixing ref-counting memtables --- db/memtablelist.cc | 10 +++++++--- db/memtablelist.h | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/db/memtablelist.cc b/db/memtablelist.cc index 884e326aa..ca662569f 100644 --- a/db/memtablelist.cc +++ b/db/memtablelist.cc @@ -76,17 +76,16 @@ void MemTableListVersion::AddIterators(const ReadOptions& options, } } +// caller is responsible for referencing m void MemTableListVersion::Add(MemTable* m) { assert(refs_ == 1); // only when refs_ == 1 is MemTableListVersion mutable - m->Ref(); memlist_.push_front(m); ++size_; } +// caller is responsible for unreferencing m void MemTableListVersion::Remove(MemTable* m) { assert(refs_ == 1); // only when refs_ == 1 is MemTableListVersion mutable - MemTable* x __attribute__((unused)) = m->Unref(); - assert(x == nullptr); // it still needs to be alive! memlist_.remove(m); --size_; } @@ -232,6 +231,11 @@ Status MemTableList::InstallMemtableFlushResults( void MemTableList::Add(MemTable* m) { assert(current_->size_ >= num_flush_not_started_); InstallNewVersion(); + // this method is used to move mutable memtable into an immutable list. + // since mutable memtable is already refcounted by the DBImpl, + // and when moving to the imutable list we don't unref it, + // we don't have to ref the memtable here. we just take over the + // reference from the DBImpl. current_->Add(m); m->MarkImmutable(); num_flush_not_started_++; diff --git a/db/memtablelist.h b/db/memtablelist.h index 16bb49743..2a2b61408 100644 --- a/db/memtablelist.h +++ b/db/memtablelist.h @@ -40,12 +40,12 @@ class MemTableListVersion { void AddIterators(const ReadOptions& options, std::vector* iterator_list); + private: // REQUIRE: m is mutable memtable void Add(MemTable* m); // REQUIRE: m is mutable memtable void Remove(MemTable* m); - private: friend class MemTableList; std::list memlist_; int size_ = 0; From 6c2ca1d3e623900d8403181bc617753ea7cb99b3 Mon Sep 17 00:00:00 2001 From: Igor Canadi Date: Mon, 27 Jan 2014 09:59:00 -0800 Subject: [PATCH 3/3] Move NeedsCompaction() from VersionSet to Version Summary: There is no reason to have functions NeedCompaction(), MaxCompactionScore() and MaxCompactionScoreLevel() in VersionSet, since they don't access any data in VersionSet. Test Plan: make check Reviewers: kailiu, haobo, sdong Reviewed By: kailiu CC: leveldb Differential Revision: https://reviews.facebook.net/D15333 --- db/db_impl.cc | 18 ++++++++---------- db/version_set.cc | 22 ++++++++++++++++++++++ db/version_set.h | 45 +++++++++------------------------------------ 3 files changed, 39 insertions(+), 46 deletions(-) diff --git a/db/db_impl.cc b/db/db_impl.cc index 0bed3bc28..d84e51699 100644 --- a/db/db_impl.cc +++ b/db/db_impl.cc @@ -1776,7 +1776,7 @@ void DBImpl::MaybeScheduleFlushOrCompaction() { // max_background_compactions hasn't been reached and, in case // bg_manual_only_ > 0, if it's a manual compaction. if ((manual_compaction_ || - versions_->NeedsCompaction() || + versions_->current()->NeedsCompaction() || (is_flush_pending && (options_.max_background_flushes <= 0))) && bg_compaction_scheduled_ < options_.max_background_compactions && (!bg_manual_only_ || manual_compaction_)) { @@ -3350,12 +3350,11 @@ Status DBImpl::MakeRoomForWrite(bool force, RecordTick(options_.statistics.get(), STALL_L0_NUM_FILES_MICROS, stall); stall_level0_num_files_ += stall; stall_level0_num_files_count_++; - } else if ( - allow_hard_rate_limit_delay && - options_.hard_rate_limit > 1.0 && - (score = versions_->MaxCompactionScore()) > options_.hard_rate_limit) { + } else if (allow_hard_rate_limit_delay && options_.hard_rate_limit > 1.0 && + (score = versions_->current()->MaxCompactionScore()) > + options_.hard_rate_limit) { // Delay a write when the compaction score for any level is too large. - int max_level = versions_->MaxCompactionScoreLevel(); + int max_level = versions_->current()->MaxCompactionScoreLevel(); mutex_.Unlock(); uint64_t delayed; { @@ -3377,10 +3376,9 @@ Status DBImpl::MakeRoomForWrite(bool force, allow_hard_rate_limit_delay = false; } mutex_.Lock(); - } else if ( - allow_soft_rate_limit_delay && - options_.soft_rate_limit > 0.0 && - (score = versions_->MaxCompactionScore()) > options_.soft_rate_limit) { + } else if (allow_soft_rate_limit_delay && options_.soft_rate_limit > 0.0 && + (score = versions_->current()->MaxCompactionScore()) > + options_.soft_rate_limit) { // Delay a write when the compaction score for any level is too large. // TODO: add statistics mutex_.Unlock(); diff --git a/db/version_set.cc b/db/version_set.cc index db5f9151a..145a99114 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -756,6 +756,28 @@ void Version::Unref() { } } +bool Version::NeedsCompaction() const { + if (file_to_compact_ != nullptr) { + return true; + } + // In universal compaction case, this check doesn't really + // check the compaction condition, but checks num of files threshold + // only. We are not going to miss any compaction opportunity + // but it's likely that more compactions are scheduled but + // ending up with nothing to do. We can improve it later. + // TODO(sdong): improve this function to be accurate for universal + // compactions. + int num_levels_to_check = + (vset_->options_->compaction_style != kCompactionStyleUniversal) ? + NumberLevels() - 1 : 1; + for (int i = 0; i < num_levels_to_check; i++) { + if (compaction_score_[i] >= 1) { + return true; + } + } + return false; +} + bool Version::OverlapInLevel(int level, const Slice* smallest_user_key, const Slice* largest_user_key) { diff --git a/db/version_set.h b/db/version_set.h index b0922d319..6b91355f7 100644 --- a/db/version_set.h +++ b/db/version_set.h @@ -99,6 +99,15 @@ class Version { void Ref(); void Unref(); + // Returns true iff some level needs a compaction. + bool NeedsCompaction() const; + + // Returns the maxmimum compaction score for levels 1 to max + double MaxCompactionScore() const { return max_compaction_score_; } + + // See field declaration + int MaxCompactionScoreLevel() const { return max_compaction_score_level_; } + void GetOverlappingInputs( int level, const InternalKey* begin, // nullptr means before all keys @@ -368,42 +377,6 @@ class VersionSet { // The caller should delete the iterator when no longer needed. Iterator* MakeInputIterator(Compaction* c); - // Returns true iff some level needs a compaction because it has - // exceeded its target size. - bool NeedsSizeCompaction() const { - // In universal compaction case, this check doesn't really - // check the compaction condition, but checks num of files threshold - // only. We are not going to miss any compaction opportunity - // but it's likely that more compactions are scheduled but - // ending up with nothing to do. We can improve it later. - // TODO: improve this function to be accurate for universal - // compactions. - int num_levels_to_check = - (options_->compaction_style != kCompactionStyleUniversal) ? - NumberLevels() - 1 : 1; - for (int i = 0; i < num_levels_to_check; i++) { - if (current_->compaction_score_[i] >= 1) { - return true; - } - } - return false; - } - // Returns true iff some level needs a compaction. - bool NeedsCompaction() const { - return ((current_->file_to_compact_ != nullptr) || - NeedsSizeCompaction()); - } - - // Returns the maxmimum compaction score for levels 1 to max - double MaxCompactionScore() const { - return current_->max_compaction_score_; - } - - // See field declaration - int MaxCompactionScoreLevel() const { - return current_->max_compaction_score_level_; - } - // Add all files listed in any live version to *live. void AddLiveFiles(std::vector* live_list);