Consolidate file cutting logic in compaction loop #1832

ajkr · 2017-02-02T20:10:24Z

It was really annoying to have two places (top and bottom of compaction loop) where we cut output files. I had bugs in both DeleteRange and dictionary compression due to updating only one of the two. This diff consolidates the file-cutting logic to the bottom of the compaction loop.

Keep in mind that my goal with input_status is to be consistent with the past behavior, even though I'm not sure it's ideal.

Test Plan:

Commands:

fill db with ~10 L0 files:
bpl=10485760;overlap=10;mcz=2;del=300000000;levels=2;ctrig=10000000; delay=10000000; stop=10000000; wbn=2; mbc=20; mb=1073741824;wbs=640000000; dds=1; sync=0; r=10000000; t=1; vs=800; bs=65536; cs=1048576; of=500000; si=1000000; ./db_bench --benchmarks=fillrandom --disable_seek_compaction=1 --mmap_read=0 --num=$r --threads=$t --value_size=$vs --block_size=$bs --cache_size=$cs --bloom_bits=10 --cache_numshardbits=4 --open_files=$of --verify_checksum=1 --db=/data/del_range_bench --sync=$sync --disable_wal=1 --compression_type=zlib --compression_ratio=0.5 --disable_data_sync=$dds --write_buffer_size=$wbs --target_file_size_base=$mb --max_write_buffer_number=$wbn --max_background_compactions=$mbc --level0_file_num_compaction_trigger=$ctrig --level0_slowdown_writes_trigger=$delay --level0_stop_writes_trigger=$stop --num_levels=$levels --delete_obsolete_files_period_micros=$del --min_level_to_compress=$mcz --max_bytes_for_level_base=$bpl --memtablerep=vector --use_existing_db=0 --disable_auto_compactions=1
compact db to L1:
bpl=10485760;overlap=10;mcz=2;del=300000000;levels=2;ctrig=10000000; delay=10000000; stop=10000000; wbn=30; mbc=20; mb=1073741824;wbs=268435456; dds=1; sync=0; r=10000000; t=1; vs=800; bs=65536; cs=1048576; of=500000; si=1000000; /usr/bin/time ./db_bench --benchmarks=compact --disable_seek_compaction=1 --mmap_read=0 --num=$r --threads=$t --value_size=$vs --block_size=$bs --cache_size=$cs --bloom_bits=10 --cache_numshardbits=4 --open_files=$of --verify_checksum=1 --db=/data/del_range_bench --sync=$sync --disable_wal=1 --compression_type=zlib --compression_ratio=0.5 --disable_data_sync=$dds --write_buffer_size=$wbs --target_file_size_base=$mb --max_write_buffer_number=$wbn --max_background_compactions=$mbc --level0_file_num_compaction_trigger=$ctrig --level0_slowdown_writes_trigger=$delay --level0_stop_writes_trigger=$stop --num_levels=$levels --delete_obsolete_files_period_micros=$del --min_level_to_compress=$mcz --max_bytes_for_level_base=$bpl --memtablerep=vector --use_existing_db=1 --disable_auto_compactions=1

Results before this diff:

/usr/bin/time output on compaction: 6.60user 6.64system 0:27.43elapsed 48%CPU
Compaction number output SST files: 5
Compaction total output size: 8416MB

Results after this diff:

/usr/bin/time output on compaction: 6.48user 6.95system 0:21.99elapsed 61%CPU
Compaction number output SST files: 5
Compaction total output size: 8415MB

facebook-github-bot · 2017-02-02T20:11:33Z

@ajkr updated the pull request - view changes

facebook-github-bot · 2017-02-02T20:13:38Z

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

ajkr · 2017-02-02T20:21:34Z

drawback is we no longer call ShouldStopBefore() on the very first key, which shouldn't return true, but it does update some internal counters.

edit: this is no longer the case due to my update below

facebook-github-bot · 2017-02-02T20:43:48Z

@ajkr updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-02-02T23:41:58Z

@ajkr updated the pull request - view changes - changes since last import

ajkr · 2017-02-07T19:29:36Z

ping

Wanning930 · 2017-02-07T19:35:23Z

快来review吧

yiwu-arbug · 2017-02-07T20:10:13Z

你们也是够了哈哈哈

siying

I started reviewing it but didn't finish.

siying · 2017-02-03T19:43:52Z

db/compaction_job.cc

+    // ShouldStopBefore() maintains state based on keys processed so far. The
+    // compaction loop always calls it on the "next" key, thus won't tell it the
+    // first key. So we do that here.
+    bool should_stop = sub_compact->ShouldStopBefore(


I'm not sure this function can be safely called if sub_compact->compaction->output_level() == 0

I didn't find the safety issue, but it's unnecessary to call it in the case you described, so I'll update the conditional.

siying · 2017-02-03T19:44:09Z

db/compaction_job.cc

+    // first key. So we do that here.
+    bool should_stop = sub_compact->ShouldStopBefore(
+      c_iter->key(), sub_compact->current_output_file_size);
+    assert(!should_stop);


Are you sure about the assert?

I think so, because ShouldStopBefore() checks the amount of overlap with grandparent level, excluding the current key. In this case, the current key is the first key, so there'll be no overlap.

Even if ShouldStopBefore() changes in the future to do more checks, I still think it'd be a bug for it to try ending a file before (i.e., excluding) the first key.

It may not be a bug. It's just we didn't expect it to be called before the first key.

Oh, would you rather ignore the return value? Because I think there's no reasonable way for us to handle terminating files before keys are added, while there are still keys remaining in the compaction input.

@ajkr yes I think it feels better to ignore this output. I can't say we won't hit any issue either, but it feels a better workflow to me.

siying · 2017-02-07T21:09:24Z

I also don't get why it is simpler after the refactoring. It feels more complicated to me.

ajkr · 2017-02-07T21:20:44Z

The compaction loop used to have file-terminating logic both at the top and the bottom. The one at the top was exercised only in rare situations, like when max_compaction_bytes is small and grandparent overlap is high. So if someone wants to change file-terminating logic (like for deleterange or compression dictionary), they have to remember to change it in two places, one of which is easy to miss. So after making this mistake in both deleterange and compression dictionary, I did this refactoring to make such mistakes impossible.

siying · 2017-02-07T21:23:18Z

@ajkr I must have missed something. I saw you just moved one FinishCompactionOutputFile() to another place. So the number of FinishCompactionOutputFile() calls didn't change.

siying · 2017-02-07T21:41:03Z

db/compaction_job.cc

+    // first key. So we do that here.
+    bool should_stop = sub_compact->ShouldStopBefore(
+      c_iter->key(), sub_compact->current_output_file_size);
+    assert(!should_stop);


@ajkr yes I think it feels better to ignore this output. I can't say we won't hit any issue either, but it feels a better workflow to me.

siying · 2017-02-07T21:44:44Z

db/compaction_job.cc

-      CompactionIterationStats range_del_out_stats;
-      status = FinishCompactionOutputFile(input->status(), sub_compact,
-                                          range_del_agg.get(),
-                                          &range_del_out_stats, &key);


I'm not sure I understand correctly.
Here we predict the grandparent size for the file after adding the key, before we add it. After remove this check, we only check after inserting the key. Is it correct?

Yes, the existing behavior is as you described. But it should be the same afterwards since I moved the "c_iter->Next();" just above the ShouldStopBefore(), so it is still doing the check before the next key has been added.

facebook-github-bot · 2017-02-08T17:08:09Z

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

facebook-github-bot · 2017-02-08T17:08:24Z

@ajkr updated the pull request - view changes - changes since last import

siying

It's hard to think of all the possible problems. If we need to add unit tests to verify it, please add. Also maybe observe the behavior of some normal benchmark running to make sure the behavior is the same.

ajkr · 2017-02-08T20:28:39Z

Hard to write unit tests because the goal is no functional change. Thanks for reviewing, will run a few benchmarks.

facebook-github-bot · 2017-02-09T00:02:19Z

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

facebook-github-bot added the CLA Signed label Feb 2, 2017

ajkr added 3 commits February 2, 2017 15:41

Consolidate file cutting logic in compaction loop

9f8c98e

use input_status

15f2cf5

ShouldStopBefore on first key

37c5262

ajkr force-pushed the refactor-compaction-loop branch from 5b2c0e6 to 37c5262 Compare February 2, 2017 23:41

siying reviewed Feb 7, 2017

View reviewed changes

modify first ShouldStopBefore call

e210a9a

siying approved these changes Feb 8, 2017

View reviewed changes

facebook-github-bot closed this in b48e477 Feb 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate file cutting logic in compaction loop #1832

Consolidate file cutting logic in compaction loop #1832

ajkr commented Feb 2, 2017 •

edited

Loading

facebook-github-bot commented Feb 2, 2017

facebook-github-bot commented Feb 2, 2017

ajkr commented Feb 2, 2017 •

edited

Loading

facebook-github-bot commented Feb 2, 2017

facebook-github-bot commented Feb 2, 2017

ajkr commented Feb 7, 2017

Wanning930 commented Feb 7, 2017

yiwu-arbug commented Feb 7, 2017

siying left a comment

siying Feb 3, 2017

ajkr Feb 8, 2017

siying Feb 3, 2017

ajkr Feb 7, 2017

siying Feb 7, 2017

ajkr Feb 7, 2017

siying Feb 7, 2017

siying commented Feb 7, 2017

ajkr commented Feb 7, 2017

siying commented Feb 7, 2017

siying Feb 7, 2017

siying Feb 7, 2017

ajkr Feb 8, 2017 •

edited

Loading

facebook-github-bot commented Feb 8, 2017

facebook-github-bot commented Feb 8, 2017

siying left a comment

ajkr commented Feb 8, 2017

facebook-github-bot commented Feb 9, 2017

Consolidate file cutting logic in compaction loop #1832

Consolidate file cutting logic in compaction loop #1832

Conversation

ajkr commented Feb 2, 2017 • edited Loading

facebook-github-bot commented Feb 2, 2017

facebook-github-bot commented Feb 2, 2017

ajkr commented Feb 2, 2017 • edited Loading

facebook-github-bot commented Feb 2, 2017

facebook-github-bot commented Feb 2, 2017

ajkr commented Feb 7, 2017

Wanning930 commented Feb 7, 2017

yiwu-arbug commented Feb 7, 2017

siying 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siying commented Feb 7, 2017

ajkr commented Feb 7, 2017

siying commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajkr Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Feb 8, 2017

facebook-github-bot commented Feb 8, 2017

siying left a comment

Choose a reason for hiding this comment

ajkr commented Feb 8, 2017

facebook-github-bot commented Feb 9, 2017

ajkr commented Feb 2, 2017 •

edited

Loading

ajkr commented Feb 2, 2017 •

edited

Loading

ajkr Feb 8, 2017 •

edited

Loading