Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce scope of compression dictionary to single SST #4952

Closed
wants to merge 12 commits into from

Conversation

ajkr
Copy link
Contributor

@ajkr ajkr commented Feb 6, 2019

Our previous approach was to train one compression dictionary per compaction, using the first output SST to train a dictionary, and then applying it on subsequent SSTs in the same compaction. While this was great for minimizing CPU/memory/I/O overhead, it did not achieve good compression ratios in practice. In our most promising potential use case, moderate reductions in a dictionary's scope make a major difference on compression ratio.

So, this PR changes compression dictionary to be scoped per-SST. It accepts the tradeoff during table building to use more memory and CPU. Important changes include:

  • The BlockBasedTableBuilder has a new state when dictionary compression is in-use: kBuffered. In that state it accumulates uncompressed data in-memory whenever Add is called.
  • After accumulating target file size bytes or calling BlockBasedTableBuilder::Finish, a BlockBasedTableBuilder moves to the kUnbuffered state. The transition (EnterUnbuffered()) involves sampling the buffered data, training a dictionary, and compressing/writing out all buffered data. In the kUnbuffered state, a BlockBasedTableBuilder behaves the same as before -- blocks are compressed/written out as soon as they fill up.
  • Samples are now whole uncompressed data blocks, except the final sample may be a partial data block so we don't breach the user's configured max_dict_bytes or zstd_max_train_bytes. The dictionary trainer is supposed to work better when we pass it real units of compression. Previously we were passing 64-byte KV samples which was not realistic.

Test Plan:

  • new unit test to verify there is a dictionary specific to each bottom-level SST
  • stress test for various configs of max_dict_bytes and zstd_max_train_bytes:
$ for train in 0 1 1024 4096 524288 4194304 ; do for dict in 0 256 4096 524288 4194304 ; do mkdir -p /data/compaction_bench/train_${train}-dict_${dict}/ && TEST_TMPDIR=/data/compaction_bench/train_${train}-dict_${dict}/ python tools/db_crashtest.py blackbox --simple --write_buffer_size=1048576 --target_file_size_base=1048576 --target_file_size_multiplier=1 --max_bytes_for_level_base=4194304 --compression_type=zstd --duration=120 --max_key=10000000 --interval=30 --compression_zstd_max_train_bytes=$train --compression_max_dict_bytes=$dict --value_size_mult=33 --writepercent=10 --readpercent=50 --max_background_compactions=8 || break; done ; done
  • shadow tested myrocks udb data and found compression ratio improvement

@ajkr ajkr requested review from riversand963 and siying and removed request for riversand963 February 6, 2019 01:04
@ajkr
Copy link
Contributor Author

ajkr commented Feb 6, 2019

This does not include charging memory usage to block cache. I need to catch up on testing before adding another feature. Hopefully we can add that feature in a separate PR.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ajkr
Copy link
Contributor Author

ajkr commented Feb 7, 2019

I am not sure about automated test. We can try something like the following, though I find it hard to imagine it'll be worth its complexity/maintenance.

  • Set target file size 1MB
  • Repeat three times: Insert 1000 1KB values of the same random string; insert 1000 1KB values of completely random data
  • Run manual compaction. Before the compaction there should be six files. After it there should be three files because the dictionaries should be super-effective at compressing the repeated random strings.
  • Also do some reads and check each of the files has a dictionary (e.g., a full scan should increment compression dict access count by three).

Copy link
Contributor

@riversand963 riversand963 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except for a few minor comments. Thanks @ajkr for the improvement.

table/block_based_table_builder.cc Outdated Show resolved Hide resolved
util/compression.h Show resolved Hide resolved
table/block_based_table_builder.h Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@facebook-github-bot
Copy link
Contributor

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

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@facebook-github-bot
Copy link
Contributor

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

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@facebook-github-bot
Copy link
Contributor

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

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants