From fcd5c5e8289d88f1100d24e7331020ee3d280464 Mon Sep 17 00:00:00 2001 From: Igor Canadi Date: Wed, 19 Mar 2014 16:52:26 -0700 Subject: [PATCH] ComputeCompactionScore in CompactionPicker Summary: As it turns out, we need the call to ComputeCompactionScore (previously: Finalize) in CompactionPicker. The issue caused a deadlock in db_stress: http://ci-builds.fb.com/job/rocksdb_crashtest/290/console The last two lines before a deadlock were: 2014/03/18-22:43:41.481029 7facafbee700 (Original Log Time 2014/03/18-22:43:41.480989) Compaction nothing to do 2014/03/18-22:43:41.481041 7faccf7fc700 wait for fewer level0 files... "Compaction nothing to do" and other thread waiting for fewer level0 files. Hm hm. I moved the pre-sorting to SaveTo, which should fix both the original and the new issue. Test Plan: make check for now, will run db_stress in jenkins Reviewers: dhruba, haobo, sdong Reviewed By: haobo CC: leveldb Differential Revision: https://reviews.facebook.net/D17037 --- db/compaction_picker.cc | 6 ++++++ db/version_set.cc | 29 +++++++++++++---------------- db/version_set.h | 6 +++--- 3 files changed, 22 insertions(+), 19 deletions(-) diff --git a/db/compaction_picker.cc b/db/compaction_picker.cc index e1b61d7e7..ccdbce72b 100644 --- a/db/compaction_picker.cc +++ b/db/compaction_picker.cc @@ -377,6 +377,12 @@ Compaction* LevelCompactionPicker::PickCompaction(Version* version, Compaction* c = nullptr; int level = -1; + // Compute the compactions needed. It is better to do it here + // and also in LogAndApply(), otherwise the values could be stale. + std::vector size_being_compacted(NumberLevels() - 1); + SizeBeingCompacted(size_being_compacted); + version->ComputeCompactionScore(size_being_compacted); + // We prefer compactions triggered by too much data in a level over // the compactions triggered by seeks. // diff --git a/db/version_set.cc b/db/version_set.cc index f8cbbf52f..70da5b9d5 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -461,7 +461,6 @@ Version::Version(VersionSet* vset, uint64_t version_number) prev_(this), refs_(0), num_levels_(vset->num_levels_), - finalized_(false), files_(new std::vector[num_levels_]), files_by_size_(num_levels_), next_file_to_compact_by_size_(num_levels_), @@ -479,7 +478,6 @@ void Version::Get(const ReadOptions& options, GetStats* stats, const Options& db_options, bool* value_found) { - assert(finalized_); Slice ikey = k.internal_key(); Slice user_key = k.user_key(); const Comparator* ucmp = vset_->icmp_.user_comparator(); @@ -643,16 +641,8 @@ bool Version::UpdateStats(const GetStats& stats) { return false; } -void Version::Finalize(std::vector& size_being_compacted) { - assert(!finalized_); - finalized_ = true; - // Pre-sort level0 for Get() - if (vset_->options_->compaction_style == kCompactionStyleUniversal) { - std::sort(files_[0].begin(), files_[0].end(), NewestFirstBySeqNo); - } else { - std::sort(files_[0].begin(), files_[0].end(), NewestFirst); - } - +void Version::ComputeCompactionScore( + std::vector& size_being_compacted) { double max_score = 0; int max_score_level = 0; @@ -1398,6 +1388,13 @@ class VersionSet::Builder { } } + // TODO(icanadi) do it in the loop above, which already sorts the files + // Pre-sort level0 for Get() + if (v->vset_->options_->compaction_style == kCompactionStyleUniversal) { + std::sort(v->files_[0].begin(), v->files_[0].end(), NewestFirstBySeqNo); + } else { + std::sort(v->files_[0].begin(), v->files_[0].end(), NewestFirst); + } CheckConsistency(v); } @@ -1575,9 +1572,9 @@ Status VersionSet::LogAndApply(VersionEdit* edit, port::Mutex* mu, } } - // The calls to Finalize and UpdateFilesBySize are cpu-heavy + // The calls to ComputeCompactionScore and UpdateFilesBySize are cpu-heavy // and is best called outside the mutex. - v->Finalize(size_being_compacted); + v->ComputeCompactionScore(size_being_compacted); v->UpdateFilesBySize(); // Write new record to MANIFEST log @@ -1870,7 +1867,7 @@ Status VersionSet::Recover() { // Install recovered version std::vector size_being_compacted(v->NumberLevels() - 1); compaction_picker_->SizeBeingCompacted(size_being_compacted); - v->Finalize(size_being_compacted); + v->ComputeCompactionScore(size_being_compacted); manifest_file_size_ = manifest_file_size; AppendVersion(v); @@ -2074,7 +2071,7 @@ Status VersionSet::DumpManifest(Options& options, std::string& dscname, // Install recovered version std::vector size_being_compacted(v->NumberLevels() - 1); compaction_picker_->SizeBeingCompacted(size_being_compacted); - v->Finalize(size_being_compacted); + v->ComputeCompactionScore(size_being_compacted); AppendVersion(v); manifest_file_number_ = next_file; diff --git a/db/version_set.h b/db/version_set.h index 39bb7d414..7d7cdf4fc 100644 --- a/db/version_set.h +++ b/db/version_set.h @@ -94,8 +94,9 @@ class Version { // Updates internal structures that keep track of compaction scores // We use compaction scores to figure out which compaction to do next - // Also pre-sorts level0 files for Get() - void Finalize(std::vector& size_being_compacted); + // REQUIRES: If Version is not yet saved to current_, it can be called without + // a lock. Once a version is saved to current_, call only with mutex held + void ComputeCompactionScore(std::vector& size_being_compacted); // Reference count management (so Versions do not disappear out from // under live iterators) @@ -227,7 +228,6 @@ class Version { Version* prev_; // Previous version in linked list int refs_; // Number of live refs to this version int num_levels_; // Number of levels - bool finalized_; // True if Finalized is called // List of files per level, files in each level are arranged // in increasing order of keys