Clean up VersionStorageInfo a bit #9494
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ateAccumulatedStats if update_stats flag is set
|
@ltamasi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
riversand963
approved these changes
Feb 3, 2022
| vstorage_->SetFinalized(); | ||
| } | ||
| void AddFileToVersionStorage(int level, uint32_t file_number, |
There was a problem hiding this comment.
Good catch for this unused method!
db/version_set.h
Outdated
|
|
||
| // Update num_non_empty_levels_. | ||
| void UpdateNumNonEmptyLevels(); | ||
| void PrepareAppend(const ImmutableOptions& immutable_options, |
There was a problem hiding this comment.
Inside the context of a Version which encloses a VersionStorageInfo, I find it a little confusing to say "PrepareAppend", because I think we are appending the enclosing Version, not VersionStorageInfo. How about renaming it to something like PrepareForVersionAppend()?
There was a problem hiding this comment.
Thanks for the review! PrepareForVersionAppend sgtm :)
|
@ltamasi has updated the pull request. You must reimport the pull request before landing. |
|
@ltamasi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@ltamasi has updated the pull request. You must reimport the pull request before landing. |
|
@ltamasi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
facebook-github-bot
pushed a commit
that referenced
this pull request
Feb 7, 2022
Summary: The patch builds on the refactoring done in #9494 and improves the performance of building the hash of file locations in `VersionStorageInfo` in two ways. First, the hash building is moved from `AddFile` (which is called under the DB mutex) to a separate post-processing step done as part of `PrepareForVersionAppend` (during which the mutex is *not* held). Second, the space necessary for the hash is preallocated to prevent costly reallocation/rehashing operations. These changes mitigate the overhead of the file location hash, which can be significant with certain workloads where the baseline CPU usage is low (see #9351, which is a workload where keys are sorted, WAL is turned off, the vector memtable implementation is used, and there are lots of small SST files). Fixes #9351 Pull Request resolved: #9504 Test Plan: `make check` ``` numactl --interleave=all ./db_bench --benchmarks=fillseq --allow_concurrent_memtable_write=false --level0_file_num_compaction_trigger=4 --level0_slowdown_writes_trigger=20 --level0_stop_writes_trigger=30 --max_background_jobs=8 --max_write_buffer_number=8 --db=/data/ltamasi-dbbench --wal_dir=/data/ltamasi-dbbench --num=800000000 --num_levels=8 --key_size=20 --value_size=400 --block_size=8192 --cache_size=51539607552 --cache_numshardbits=6 --compression_max_dict_bytes=0 --compression_ratio=0.5 --compression_type=lz4 --bytes_per_sync=8388608 --cache_index_and_filter_blocks=1 --cache_high_pri_pool_ratio=0.5 --benchmark_write_rate_limit=0 --write_buffer_size=16777216 --target_file_size_base=16777216 --max_bytes_for_level_base=67108864 --verify_checksum=1 --delete_obsolete_files_period_micros=62914560 --max_bytes_for_level_multiplier=8 --statistics=0 --stats_per_interval=1 --stats_interval_seconds=20 --histogram=1 --bloom_bits=10 --open_files=-1 --subcompactions=1 --compaction_style=0 --level_compaction_dynamic_level_bytes=true --pin_l0_filter_and_index_blocks_in_cache=1 --soft_pending_compaction_bytes_limit=167503724544 --hard_pending_compaction_bytes_limit=335007449088 --min_level_to_compress=0 --use_existing_db=0 --sync=0 --threads=1 --memtablerep=vector --disable_wal=1 --seed=<some_seed> ``` Final statistics before this patch: ``` Cumulative writes: 0 writes, 697M keys, 0 commit groups, 0.0 writes per commit group, ingest: 283.25 GB, 241.08 MB/s Interval writes: 0 writes, 1264K keys, 0 commit groups, 0.0 writes per commit group, ingest: 525.69 MB, 176.67 MB/s ``` With the patch: ``` Cumulative writes: 0 writes, 759M keys, 0 commit groups, 0.0 writes per commit group, ingest: 308.57 GB, 262.63 MB/s Interval writes: 0 writes, 1555K keys, 0 commit groups, 0.0 writes per commit group, ingest: 646.61 MB, 215.11 MB/s ``` Reviewed By: riversand963 Differential Revision: D34014734 Pulled By: ltamasi fbshipit-source-id: acb2703677451d5ccaa7e9d950844b33d240695b
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
The patch does some cleanup in and around
VersionStorageInfo:PrepareApplytoPrepareAppendinVersionto make it clear that it is to be called before appending the
VersiontoVersionSet(viaAppendVersion), not before applying anyVersionEdits.VersionStorageInfo::PrepareForVersionAppend(called by
Version::PrepareAppend) that encapsulates the population of thevarious derived data structures in
VersionStorageInfo, and turns themethods computing the derived structures (
UpdateNumNonEmptyLevels,CalculateBaseBytesetc.) into private helpers.Version::PrepareAppendso it only callsUpdateAccumulatedStatsif the
update_statsflag is set. (Earlier, this was checked by the callee.)Related to this, it also moves the call to
ComputeCompensatedSizestoVersionStorageInfo::PrepareForVersionAppend.version_builder_test,version_set_test, andcompaction_picker_testsoPrepareForVersionAppendis called anytimea new
VersionStorageInfois set up or saved. This cleanup also involvessplitting
VersionStorageInfoTest.MaxBytesForLevelDynamicinto multiple smaller test cases.
Test Plan:
Ran
make checkand the crash test script for a while.