From b42ceb9598c5210056fb85ad979e2eb488d61888 Mon Sep 17 00:00:00 2001 From: Igor Canadi Date: Mon, 7 Apr 2014 14:21:25 -0700 Subject: [PATCH] Simplify cleanup of dead (refcount == 0) column families --- db/column_family.cc | 14 ++++++++++++++ db/column_family.h | 12 ++++++++---- db/db_filesnapshot.cc | 10 +++------- db/db_impl.cc | 45 +++++++++++++------------------------------ 4 files changed, 38 insertions(+), 43 deletions(-) diff --git a/db/column_family.cc b/db/column_family.cc index ed50d0fef..45ea22c23 100644 --- a/db/column_family.cc +++ b/db/column_family.cc @@ -428,6 +428,20 @@ void ColumnFamilySet::Lock() { void ColumnFamilySet::Unlock() { spin_lock_.clear(std::memory_order_release); } +// REQUIRES: DB mutex held +void ColumnFamilySet::FreeDeadColumnFamilies() { + autovector to_delete; + for (auto cfd = dummy_cfd_->next_; cfd != dummy_cfd_; cfd = cfd->next_) { + if (cfd->refs_ == 0) { + to_delete.push_back(cfd); + } + } + for (auto cfd : to_delete) { + // this is very rare, so it's not a problem that we do it under a mutex + delete cfd; + } +} + // under a DB mutex void ColumnFamilySet::RemoveColumnFamily(ColumnFamilyData* cfd) { auto cfd_iter = column_family_data_.find(cfd->GetID()); diff --git a/db/column_family.h b/db/column_family.h index 01d76734f..42e02f07d 100644 --- a/db/column_family.h +++ b/db/column_family.h @@ -91,8 +91,7 @@ struct SuperVersion { SuperVersion() = default; ~SuperVersion(); SuperVersion* Ref(); - // Returns true if this was the last reference and caller should - // call Clenaup() and delete the object + bool Unref(); // call these two methods with db mutex held @@ -133,8 +132,9 @@ class ColumnFamilyData { void Ref() { ++refs_; } // will just decrease reference count to 0, but will not delete it. returns - // true if the ref count was decreased to zero and needs to be cleaned up by - // the caller + // true if the ref count was decreased to zero. in that case, it can be + // deleted by the caller immediatelly, or later, by calling + // FreeDeadColumnFamilies() bool Unref() { assert(refs_ > 0); return --refs_ == 0; @@ -343,6 +343,10 @@ class ColumnFamilySet { void Lock(); void Unlock(); + // REQUIRES: DB mutex held + // Don't call while iterating over ColumnFamilySet + void FreeDeadColumnFamilies(); + private: friend class ColumnFamilyData; // helper function that gets called from cfd destructor diff --git a/db/db_filesnapshot.cc b/db/db_filesnapshot.cc index bdb443dd0..61a818465 100644 --- a/db/db_filesnapshot.cc +++ b/db/db_filesnapshot.cc @@ -67,23 +67,19 @@ Status DBImpl::GetLiveFiles(std::vector& ret, if (flush_memtable) { // flush all dirty data to disk. - autovector to_delete; Status status; for (auto cfd : *versions_->GetColumnFamilySet()) { cfd->Ref(); mutex_.Unlock(); status = FlushMemTable(cfd, FlushOptions()); mutex_.Lock(); - if (cfd->Unref()) { - to_delete.push_back(cfd); - } + cfd->Unref(); if (!status.ok()) { break; } } - for (auto cfd : to_delete) { - delete cfd; - } + versions_->GetColumnFamilySet()->FreeDeadColumnFamilies(); + if (!status.ok()) { mutex_.Unlock(); Log(options_.info_log, "Cannot Flush data %s\n", diff --git a/db/db_impl.cc b/db/db_impl.cc index ca5141c72..2427f63ba 100644 --- a/db/db_impl.cc +++ b/db/db_impl.cc @@ -404,21 +404,16 @@ DBImpl::DBImpl(const DBOptions& options, const std::string& dbname) DBImpl::~DBImpl() { mutex_.Lock(); if (flush_on_destroy_) { - autovector to_delete; for (auto cfd : *versions_->GetColumnFamilySet()) { if (cfd->mem()->GetFirstSequenceNumber() != 0) { cfd->Ref(); mutex_.Unlock(); FlushMemTable(cfd, FlushOptions()); mutex_.Lock(); - if (cfd->Unref()) { - to_delete.push_back(cfd); - } + cfd->Unref(); } } - for (auto cfd : to_delete) { - delete cfd; - } + versions_->GetColumnFamilySet()->FreeDeadColumnFamilies(); } // Wait for background work to finish @@ -1941,7 +1936,6 @@ Status DBImpl::BackgroundFlush(bool* madeProgress, // flushing one column family reports a failure, we will continue flushing // other column families. however, call_status will be a failure in that case. Status call_status; - autovector to_delete; // refcounting in iteration for (auto cfd : *versions_->GetColumnFamilySet()) { cfd->Ref(); @@ -1958,13 +1952,9 @@ Status DBImpl::BackgroundFlush(bool* madeProgress, if (call_status.ok() && !flush_status.ok()) { call_status = flush_status; } - if (cfd->Unref()) { - to_delete.push_back(cfd); - } - } - for (auto cfd : to_delete) { - delete cfd; + cfd->Unref(); } + versions_->GetColumnFamilySet()->FreeDeadColumnFamilies(); return call_status; } @@ -2105,6 +2095,8 @@ void DBImpl::BackgroundCallCompaction() { MaybeScheduleLogDBDeployStats(); + versions_->GetColumnFamilySet()->FreeDeadColumnFamilies(); + // Previous compaction may have produced too many files in a level, // So reschedule another compaction if we made progress in the // last compaction. @@ -2139,7 +2131,6 @@ Status DBImpl::BackgroundCompaction(bool* madeProgress, } // FLUSH preempts compaction - autovector to_delete; Status flush_stat; for (auto cfd : *versions_->GetColumnFamilySet()) { while (cfd->imm()->IsFlushPending()) { @@ -2151,9 +2142,7 @@ Status DBImpl::BackgroundCompaction(bool* madeProgress, cfd->Ref(); flush_stat = FlushMemTableToOutputFile(cfd, madeProgress, deletion_state, log_buffer); - if (cfd->Unref()) { - to_delete.push_back(cfd); - } + cfd->Unref(); if (!flush_stat.ok()) { if (is_manual) { manual_compaction_->status = flush_stat; @@ -2161,18 +2150,9 @@ Status DBImpl::BackgroundCompaction(bool* madeProgress, manual_compaction_->in_progress = false; manual_compaction_ = nullptr; } - break; + return flush_stat; } } - if (!flush_stat.ok()) { - break; - } - } - for (auto cfd : to_delete) { - delete cfd; - } - if (!flush_stat.ok()) { - return flush_stat; } unique_ptr c; @@ -3840,22 +3820,23 @@ Status DBImpl::Write(const WriteOptions& options, WriteBatch* my_batch) { } Status status; - autovector to_delete; // refcounting cfd in iteration + bool dead_cfd = false; for (auto cfd : *versions_->GetColumnFamilySet()) { cfd->Ref(); // May temporarily unlock and wait. status = MakeRoomForWrite(cfd, my_batch == nullptr); if (cfd->Unref()) { - to_delete.push_back(cfd); + dead_cfd = true; } if (!status.ok()) { break; } } - for (auto cfd : to_delete) { - delete cfd; + if (dead_cfd) { + versions_->GetColumnFamilySet()->FreeDeadColumnFamilies(); } + uint64_t last_sequence = versions_->LastSequence(); Writer* last_writer = &w; if (status.ok() && my_batch != nullptr) { // nullptr batch is for compactions