Avoid double-compacting data in bottom level in manual compactions #5138

miasantreble · 2019-04-02T20:43:08Z

Depending on the config, manual compaction (leveled compaction style) does following compactions:
L0->L1
L1->L2
...
Ln-1 -> Ln
Ln -> Ln
The final Ln -> Ln compaction is partly unnecessary as it recompacts all the files that were just generated by the Ln-1 -> Ln. We should avoid recompacting such files. This rule should be applied to Lmax only.
Resolves issue #4995

miasantreble · 2019-04-02T20:44:29Z

Hey @ajkr , do you mind taking a quick look to see if this approach makes sense? Thanks!

ajkr · 2019-04-02T23:09:57Z

Thanks for doing this, will take a look. BTW, do you mind reminding me what @yoshinorim's reason was for only operating on the bottom level? I remember he wrote some insightful reason on the task.

ajkr · 2019-04-02T23:14:35Z

Never mind, I think it makes sense. We wouldn't want to skip files in middle levels because the latest data for the range (as of the time CompactRange is called) might not be included by other compactions happening in parallel. So just because file number is higher in those levels doesn't necessarily mean the newest data has been pushed down.

ajkr · 2019-04-02T23:31:22Z

db/compaction_picker.cc

+    if (inputs_shrinked.empty()) {
+      return nullptr;
+    }
+    inputs.files.swap(inputs_shrinked);


Is it a problem that the files may not cover a contiguous key-range after this code filters some out?

I wonder if we should pick each compaction consisting of one contiguous range of files that all need to be compacted. We can then set compaction_end to the end of that range and another call will be made for the next contiguous range, etc.

Hm, it looks like the following ExpandInputsToCleanCut refills any holes if we pick non-contiguous files here. It seems safe, but OTOH some cases may be suboptimal. For example, the first and last file might be the only ones that need compaction (i.e., inputs_shrinked contains only those two). But then ExpandInputsToCleanCut will repopulate the input files with everything in the range, which leaves us with no files skipped.

Thanks @ajkr !
let me see if I understood the idea correctly:
so in CompactionPicker::CompactRange, after the call to ExpandInputsToCleanCut and SetupOtherInputs, we will still filter out new files larger than max_sst_file_number, then maybe call a new function to break up inputs_shrinked into continuous ranges of files, and for each range, call SetupOtherInputs and then create/register one new Compaction?
This will create a number of Compaction objects but the caller DBImpl::RunManualCompaction seems to expect only one Compaction?

I guess if it's only suboptimal in some corner cases but optimizes in general cases, we can live with that. Or if really needed, we can also do some trick in ExpandInputsToCleanCut but that might be a bit error-prone

Rather than leaving holes in the middle, should we just cut the compaction?
If we have files [a b] [c d] [e f], and [c d] is newly generated, should we pick [a b] to compact itself, and then [e f] in a following one?

miasantreble · 2019-04-02T23:31:32Z

@ajkr was in a meeting earlier, but here is from @yoshinorim :

For example:
Suppose calling CompactRange(), and max sst number was 100 at that time. Currently, compactions will happen like this:
001.sst, 002.sst, 003.sst (L0) -> 101.sst (L1)
004.sst, 005.sst, 101.sst, 006.sst (L1) -> 102.sst (L2)
011.sst, 012.sst, 102.sst, 013.sst (L2) -> 103.sst (L3)
If you skip sst files > max number for all levels, several files (101.sst in L1 and 102.sst in L2 in this case) will remain in higher levels, which may waste some space. That's why I think only reasonable level to skip compaction is Lmax.

I believe it's the exact same reason that you summarized in the next comment

ajkr · 2019-04-03T00:14:31Z

I think you can return one compaction at a time consisting of a range of contiguous files. The caller (manual compaction) will have to call CompactRange again to get the next compaction of contiguous files. It can know where to search for the next range of contiguous files if we set compaction_end to the end of the current range.

…

On Tue, Apr 2, 2019, 4:48 PM Zhongyi Xie ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In db/compaction_picker.cc <#5138 (comment)>: > @@ -660,6 +661,22 @@ Compaction* CompactionPicker::CompactRange( } assert(output_path_id < static_cast<uint32_t>(ioptions_.cf_paths.size())); + // for bottom level compaction, skip files that are created during the + // current compaction + if (max_sst_file_number > 0) { + assert(input_level == output_level); + std::vector<FileMetaData*> inputs_shrinked; + for (size_t i = 0; i < inputs.size(); ++i) { + if (inputs[i]->fd.GetNumber() <= max_sst_file_number) { + inputs_shrinked.emplace_back(inputs[i]); + } + } + if (inputs_shrinked.empty()) { + return nullptr; + } + inputs.files.swap(inputs_shrinked); Thanks @ajkr <https://github.com/ajkr> ! let me see if I understood the idea correctly: so in CompactionPicker::CompactRange, after the call to GetOverlappingInputs, we will still filter out new files larger than max_sst_file_number, then maybe call a new function to break up inputs_shrinked into continuous ranges of files, and for each range, call SetupOtherInputs and then create/register one new Compaction? This will create a number of Compaction objects but the caller DBImpl::RunManualCompaction seems to expect only one Compaction? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5138 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEjxSq-gYF4CFCb4ekIBBKaR72P1W2Pcks5vc-xPgaJpZM4cZFUa> .

miasantreble · 2019-04-03T00:58:21Z

I see what you mean now, RunManualCompaction will repeatedly call manual.cfd->CompactRange until manual.done is set to true, and every time BackgroundCompaction runs, it will update begin with manual_end so I guess all we need to do now is in CompactionPicker when filtering inputs by file number, stop as soon as we find a newly created file, let ExpandInputsToCleanCut do its job and run the compaction using the truncated inputs. Will try out this idea. Thanks!

siying · 2019-04-10T23:07:56Z

@miasantreble oops, I didn't see your latest comments. I was just trying to mention the same thing as you guys concluded:)

db/compaction_picker.cc

siying · 2019-04-10T23:19:47Z

db/db_compaction_test.cc

+  num = comp_stats2[2].num_input_files_in_output_level;
+  ASSERT_EQ(num, 0);
+}
+


We have compaction_picker_test.cc. Can we have more thorough test coverage there for some corner cases?

db/db_impl_compaction_flush.cc

db/column_family.cc

siying · 2019-04-10T23:35:05Z

I gave it a pass, and I believe we are on the right track!

db/column_family.cc

db/compaction_picker.cc

siying

LGTM. Defer to @ajkr for approval.

db/db_compaction_test.cc

db/db_impl_compaction_flush.cc

facebook-github-bot

@miasantreble has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

miasantreble · 2019-04-17T00:26:44Z

@ajkr do you mind taking another look before we land the PR? Thanks!

ajkr

Makes sense to me. Nice!

ajkr · 2019-04-17T00:48:08Z

HISTORY.md

@@ -18,6 +18,7 @@
 ## 6.1.0 (3/27/2019)
 ### New Features
 * Introduce two more stats levels, kExceptHistogramOrTimers and kExceptTimers.
+* Added BottommostLevelCompaction::kForceOptimized to avoid double compacting newly compacted files in bottom level compaction of manual compaction.


I guess it should be under "Unreleased" in master branch and under "6.1.x" in 6.1.fb branch, assuming you're going to backport it. Although perhaps the protocol changed after my tenure.

Right, thanks for pointing out. The protocol hasn't changed yet :)

ajkr · 2019-04-17T01:12:21Z

HISTORY.md

@@ -18,6 +18,7 @@
 ## 6.1.0 (3/27/2019)
 ### New Features
 * Introduce two more stats levels, kExceptHistogramOrTimers and kExceptTimers.
+* Added BottommostLevelCompaction::kForceOptimized to avoid double compacting newly compacted files in bottom level compaction of manual compaction.


For my understanding, is it an option because kForce should be able to force recompaction even on files that were trivially moved to the bottom level?

Yes, we decided to make it an option because tools like option_change_migration and tests like DBCompactionTestWithParam .ConvertCompactionStyle expects manual compactions to generate one single file in the bottom level

facebook-github-bot · 2019-04-17T03:02:12Z

@miasantreble has updated the pull request. Re-import the pull request

facebook-github-bot

@miasantreble has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2019-04-17T07:44:31Z

@miasantreble merged this pull request in baa5302.

…acebook#5138) Summary: Depending on the config, manual compaction (leveled compaction style) does following compactions: L0->L1 L1->L2 ... Ln-1 -> Ln Ln -> Ln The final Ln -> Ln compaction is partly unnecessary as it recompacts all the files that were just generated by the Ln-1 -> Ln. We should avoid recompacting such files. This rule should be applied to Lmax only. Resolves issue facebook#4995 Pull Request resolved: facebook#5138 Differential Revision: D14940106 Pulled By: miasantreble fbshipit-source-id: 8d3cf5507a17e76f3333cfd4bac5256d005636e5

facebook-github-bot added the CLA Signed label Apr 2, 2019

miasantreble added the WIP Work in progress label Apr 2, 2019

ajkr reviewed Apr 2, 2019

View reviewed changes

siying reviewed Apr 10, 2019

View reviewed changes

siying reviewed Apr 11, 2019

View reviewed changes

db/column_family.cc Outdated Show resolved Hide resolved

db/compaction_picker.cc Outdated Show resolved Hide resolved

miasantreble force-pushed the bottom-level-double-compaction branch from 39ea4b9 to 1127a66 Compare April 11, 2019 23:06

miasantreble removed the WIP Work in progress label Apr 11, 2019

miasantreble added 4 commits April 14, 2019 12:34

CompactRange: avoid double-compacting data in bottom level

d3e5f6a

avoid creating holes in inputs_shrinked

f2a5e8f

add test coverage

31ab58d

address review comments

eb4856b

miasantreble force-pushed the bottom-level-double-compaction branch from 8652a22 to 9305fe1 Compare April 14, 2019 19:34

add BottommostLevelCompaction::kForceOptimized

855556d

miasantreble force-pushed the bottom-level-double-compaction branch from 9305fe1 to 855556d Compare April 14, 2019 20:32

siying reviewed Apr 15, 2019

View reviewed changes

db/db_compaction_test.cc Outdated Show resolved Hide resolved

db/db_impl_compaction_flush.cc Outdated Show resolved Hide resolved

forward CompactRangeOptions when possible

c2eeaa5

facebook-github-bot reviewed Apr 15, 2019

View reviewed changes

ajkr approved these changes Apr 17, 2019

View reviewed changes

fix history.md

2debfce

facebook-github-bot reviewed Apr 17, 2019

View reviewed changes

facebook-github-bot closed this in baa5302 Apr 17, 2019

facebook-github-bot added the Merged label Apr 17, 2019

miasantreble deleted the bottom-level-double-compaction branch April 18, 2019 21:36

ajkr mentioned this pull request Sep 29, 2021

Redundant compact most bottom level on manual compact #8556

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid double-compacting data in bottom level in manual compactions #5138

Avoid double-compacting data in bottom level in manual compactions #5138

miasantreble commented Apr 2, 2019

miasantreble commented Apr 2, 2019

ajkr commented Apr 2, 2019

ajkr commented Apr 2, 2019 •

edited

Loading

ajkr Apr 2, 2019

ajkr Apr 2, 2019

miasantreble Apr 2, 2019 •

edited

Loading

miasantreble Apr 2, 2019

siying Apr 10, 2019

miasantreble commented Apr 2, 2019 •

edited

Loading

ajkr commented Apr 3, 2019 via email

miasantreble commented Apr 3, 2019

siying commented Apr 10, 2019

siying Apr 10, 2019

siying commented Apr 10, 2019

siying left a comment

facebook-github-bot left a comment

miasantreble commented Apr 17, 2019

ajkr left a comment

ajkr Apr 17, 2019

miasantreble Apr 17, 2019

ajkr Apr 17, 2019

miasantreble Apr 17, 2019

facebook-github-bot commented Apr 17, 2019

facebook-github-bot left a comment

facebook-github-bot commented Apr 17, 2019

Avoid double-compacting data in bottom level in manual compactions #5138

Avoid double-compacting data in bottom level in manual compactions #5138

Conversation

miasantreble commented Apr 2, 2019

miasantreble commented Apr 2, 2019

ajkr commented Apr 2, 2019

ajkr commented Apr 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miasantreble Apr 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miasantreble commented Apr 2, 2019 • edited Loading

ajkr commented Apr 3, 2019 via email

miasantreble commented Apr 3, 2019

siying commented Apr 10, 2019

Choose a reason for hiding this comment

siying commented Apr 10, 2019

siying left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

miasantreble commented Apr 17, 2019

ajkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 17, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 17, 2019

ajkr commented Apr 2, 2019 •

edited

Loading

miasantreble Apr 2, 2019 •

edited

Loading

miasantreble commented Apr 2, 2019 •

edited

Loading