From 1bdfcef7bf37b4dfc171e0588bb2d0fa5385a8d0 Mon Sep 17 00:00:00 2001 From: Poornima Chozhiyath Raman Date: Mon, 27 Jul 2015 14:25:57 -0700 Subject: [PATCH] Fix when output level is 0 of universal compaction with trivial move Summary: Fix for universal compaction with trivial move, when the ouput level is 0. The tests where failing. Fixed by allowing normal compaction when output level is 0. Test Plan: modified test cases run successfully. Reviewers: sdong, yhchiang, IslamAbdelRahman Reviewed By: IslamAbdelRahman Subscribers: anthony, kradhakrishnan, leveldb, dhruba Differential Revision: https://reviews.facebook.net/D42933 --- db/compaction.cc | 3 ++- db/compaction_picker.cc | 21 +++++++++++++++------ db/compaction_picker.h | 2 +- db/db_impl.cc | 7 ++++--- db/db_universal_compaction_test.cc | 29 +++++++++++++++++++---------- util/auto_roll_logger_test.cc | 3 +-- 6 files changed, 42 insertions(+), 23 deletions(-) diff --git a/db/compaction.cc b/db/compaction.cc index e5ec7de42..0aa17c09c 100644 --- a/db/compaction.cc +++ b/db/compaction.cc @@ -168,7 +168,8 @@ bool Compaction::IsTrivialMove() const { // Used in universal compaction, where trivial move can be done if the // input files are non overlapping - if (cfd_->ioptions()->compaction_options_universal.allow_trivial_move) { + if ((cfd_->ioptions()->compaction_options_universal.allow_trivial_move) && + (output_level_ != 0)) { return is_trivial_move_; } diff --git a/db/compaction_picker.cc b/db/compaction_picker.cc index 13e3d94c2..650d77297 100644 --- a/db/compaction_picker.cc +++ b/db/compaction_picker.cc @@ -1114,7 +1114,7 @@ void UniversalCompactionPicker::SortedRun::DumpSizeInfo( std::vector UniversalCompactionPicker::CalculateSortedRuns( - const VersionStorageInfo& vstorage) { + const VersionStorageInfo& vstorage, const ImmutableCFOptions& ioptions) { std::vector ret; for (FileMetaData* f : vstorage.LevelFiles(0)) { ret.emplace_back(0, f, f->fd.GetFileSize(), f->compensated_file_size, @@ -1128,10 +1128,18 @@ UniversalCompactionPicker::CalculateSortedRuns( for (FileMetaData* f : vstorage.LevelFiles(level)) { total_compensated_size += f->compensated_file_size; total_size += f->fd.GetFileSize(); - // Compaction always includes all files for a non-zero level, so for a - // non-zero level, all the files should share the same being_compacted - // value. - assert(is_first || f->being_compacted == being_compacted); + if (ioptions.compaction_options_universal.allow_trivial_move == true) { + if (f->being_compacted) { + being_compacted = f->being_compacted; + } + } else { + // Compaction always includes all files for a non-zero level, so for a + // non-zero level, all the files should share the same being_compacted + // value. + // This assumption is only valid when + // ioptions.compaction_options_universal.allow_trivial_move is false + assert(is_first || f->being_compacted == being_compacted); + } if (is_first) { being_compacted = f->being_compacted; is_first = false; @@ -1223,7 +1231,8 @@ Compaction* UniversalCompactionPicker::PickCompaction( VersionStorageInfo* vstorage, LogBuffer* log_buffer) { const int kLevel0 = 0; double score = vstorage->CompactionScore(kLevel0); - std::vector sorted_runs = CalculateSortedRuns(*vstorage); + std::vector sorted_runs = + CalculateSortedRuns(*vstorage, ioptions_); if (sorted_runs.size() < (unsigned int)mutable_cf_options.level0_file_num_compaction_trigger) { diff --git a/db/compaction_picker.h b/db/compaction_picker.h index 65ca73abf..61ed99505 100644 --- a/db/compaction_picker.h +++ b/db/compaction_picker.h @@ -275,7 +275,7 @@ class UniversalCompactionPicker : public CompactionPicker { const std::vector& sorted_runs, LogBuffer* log_buffer); static std::vector CalculateSortedRuns( - const VersionStorageInfo& vstorage); + const VersionStorageInfo& vstorage, const ImmutableCFOptions& ioptions); // Pick a path ID to place a newly generated file, with its estimated file // size. diff --git a/db/db_impl.cc b/db/db_impl.cc index 133297c1e..3d98fbc7d 100644 --- a/db/db_impl.cc +++ b/db/db_impl.cc @@ -2542,8 +2542,7 @@ Status DBImpl::BackgroundCompaction(bool* madeProgress, JobContext* job_context, int32_t moved_files = 0; int64_t moved_bytes = 0; for (unsigned int l = 0; l < c->num_input_levels(); l++) { - if (l == static_cast(c->output_level()) || - (c->output_level() == 0)) { + if (c->level(l) == c->output_level()) { continue; } for (size_t i = 0; i < c->num_input_files(l); i++) { @@ -2591,7 +2590,9 @@ Status DBImpl::BackgroundCompaction(bool* madeProgress, JobContext* job_context, // Clear Instrument ThreadStatusUtil::ResetThreadStatus(); } else { - TEST_SYNC_POINT("DBImpl::BackgroundCompaction:NonTrivial"); + int output_level __attribute__((unused)) = c->output_level(); + TEST_SYNC_POINT_CALLBACK("DBImpl::BackgroundCompaction:NonTrivial", + &output_level); assert(is_snapshot_supported_ || snapshots_.empty()); CompactionJob compaction_job( job_context->job_id, c.get(), db_options_, env_options_, diff --git a/db/db_universal_compaction_test.cc b/db/db_universal_compaction_test.cc index cae56eab1..c788157bc 100644 --- a/db/db_universal_compaction_test.cc +++ b/db/db_universal_compaction_test.cc @@ -452,8 +452,12 @@ TEST_P(DBTestUniversalCompactionMultiLevels, UniversalCompactionTrivialMove) { "DBImpl::BackgroundCompaction:TrivialMove", [&](void* arg) { trivial_move++; }); rocksdb::SyncPoint::GetInstance()->SetCallBack( - "DBImpl::BackgroundCompaction:NonTrivial", - [&](void* arg) { non_trivial_move++; }); + "DBImpl::BackgroundCompaction:NonTrivial", [&](void* arg) { + non_trivial_move++; + ASSERT_TRUE(arg != nullptr); + int output_level = *(static_cast(arg)); + ASSERT_EQ(output_level, 0); + }); rocksdb::SyncPoint::GetInstance()->EnableProcessing(); Options options; @@ -462,7 +466,7 @@ TEST_P(DBTestUniversalCompactionMultiLevels, UniversalCompactionTrivialMove) { options.num_levels = 3; options.write_buffer_size = 100 << 10; // 100KB options.level0_file_num_compaction_trigger = 3; - options.max_background_compactions = 1; + options.max_background_compactions = 2; options.target_file_size_base = 32 * 1024; options = CurrentOptions(options); DestroyAndReopen(options); @@ -474,7 +478,7 @@ TEST_P(DBTestUniversalCompactionMultiLevels, UniversalCompactionTrivialMove) { ReopenWithColumnFamilies({"default", "pikachu"}, options); Random rnd(301); - int num_keys = 15000; + int num_keys = 150000; for (int i = 0; i < num_keys; i++) { ASSERT_OK(Put(1, Key(i), Key(i))); } @@ -484,7 +488,7 @@ TEST_P(DBTestUniversalCompactionMultiLevels, UniversalCompactionTrivialMove) { dbfull()->TEST_WaitForCompact(); ASSERT_GT(trivial_move, 0); - ASSERT_EQ(non_trivial_move, 0); + ASSERT_GT(non_trivial_move, 0); rocksdb::SyncPoint::GetInstance()->DisableProcessing(); } @@ -789,14 +793,18 @@ TEST_P(DBTestUniversalCompaction, UniversalCompactionTrivialMoveTest1) { "DBImpl::BackgroundCompaction:TrivialMove", [&](void* arg) { trivial_move++; }); rocksdb::SyncPoint::GetInstance()->SetCallBack( - "DBImpl::BackgroundCompaction:NonTrivial", - [&](void* arg) { non_trivial_move++; }); + "DBImpl::BackgroundCompaction:NonTrivial", [&](void* arg) { + non_trivial_move++; + ASSERT_TRUE(arg != nullptr); + int output_level = *(static_cast(arg)); + ASSERT_EQ(output_level, 0); + }); rocksdb::SyncPoint::GetInstance()->EnableProcessing(); Options options; options.compaction_style = kCompactionStyleUniversal; options.compaction_options_universal.allow_trivial_move = true; - options.num_levels = 3; + options.num_levels = 2; options.write_buffer_size = 100 << 10; // 100KB options.level0_file_num_compaction_trigger = 3; options.max_background_compactions = 1; @@ -811,7 +819,7 @@ TEST_P(DBTestUniversalCompaction, UniversalCompactionTrivialMoveTest1) { ReopenWithColumnFamilies({"default", "pikachu"}, options); Random rnd(301); - int num_keys = 150000; + int num_keys = 250000; for (int i = 0; i < num_keys; i++) { ASSERT_OK(Put(1, Key(i), Key(i))); } @@ -821,7 +829,7 @@ TEST_P(DBTestUniversalCompaction, UniversalCompactionTrivialMoveTest1) { dbfull()->TEST_WaitForCompact(); ASSERT_GT(trivial_move, 0); - ASSERT_EQ(non_trivial_move, 0); + ASSERT_GT(non_trivial_move, 0); rocksdb::SyncPoint::GetInstance()->DisableProcessing(); } @@ -835,6 +843,7 @@ TEST_P(DBTestUniversalCompaction, UniversalCompactionTrivialMoveTest2) { rocksdb::SyncPoint::GetInstance()->SetCallBack( "DBImpl::BackgroundCompaction:NonTrivial", [&](void* arg) { non_trivial_move++; }); + rocksdb::SyncPoint::GetInstance()->EnableProcessing(); Options options; diff --git a/util/auto_roll_logger_test.cc b/util/auto_roll_logger_test.cc index ef340630a..138eb6eb4 100644 --- a/util/auto_roll_logger_test.cc +++ b/util/auto_roll_logger_test.cc @@ -28,8 +28,7 @@ class AutoRollLoggerTest : public testing::Test { // become confused std::string testDir(kTestDir); std::replace_if(testDir.begin(), testDir.end(), - [](char ch) { return ch == '/'; }, - '\\'); + [](char ch) { return ch == '/'; }, '\\'); std::string deleteCmd = "if exist " + testDir + " rd /s /q " + testDir; #else std::string deleteCmd = "rm -rf " + kTestDir;