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

Improve FragmentTombstones() speed by lazily initializing seq_set_ #10848

Closed
wants to merge 5 commits into from

Conversation

cbi42
Copy link
Member

@cbi42 cbi42 commented Oct 24, 2022

Summary: FragmentedRangeTombstoneList has a member variable seq_set_ that contains the sequence numbers of all range tombstones in a set. The set is constructed in FragmentTombstones() and is used only in FragmentedRangeTombstoneList::ContainsRange() which only happens during compaction. This PR moves the initialization of seq_set_ to FragmentedRangeTombstoneList::ContainsRange(). This should speed up FragmentTombstones() when the range tombstone list is used for read/scan requests, this is true for most cases when FragmentTombstones() is called for range tombstones of the mutable memtable. Microbench shows the speed improvement to be ~45%.

Test plan:

  • Existing tests and stress test: python3 tools/db_crashtest.py whitebox --simple --verify_iterator_with_expected_state_one_in=5.
  • Microbench: update range_del_aggregator_bench to benchmark speed of FragmentTombstones():
./range_del_aggregator_bench --num_range_tombstones=1000 --tombstone_start_upper_bound=50000000 --num_runs=10000 --tombstone_width_mean=200 --should_deletes_per_run=100 --use_compaction_range_del_aggregator=true

Before this PR:
=========================
Fragment Tombstones:     270.286 us
AddTombstones:           1.28933 us
ShouldDelete (first):    0.525528 us
ShouldDelete (rest):     0.0797519 us

After this PR: time to fragment tombstones is pushed to AddTombstones() which only happen during compaction.
=========================
Fragment Tombstones:     149.879 us
AddTombstones:           102.131 us
ShouldDelete (first):    0.565871 us
ShouldDelete (rest):     0.0729444 us
  • db_bench: this should improve speed for fragmenting range tombstones for mutable memtable:
./db_bench --benchmarks=readwhilewriting --writes_per_range_tombstone=100 --max_write_buffer_number=100 --min_write_buffer_number_to_merge=100 --writes=500000 --reads=250000 --disable_auto_compactions --max_num_range_tombstones=100000 --finish_after_writes --write_buffer_size=1073741824 --threads=25

Before this PR: 
readwhilewriting :      18.301 micros/op 1310445 ops/sec 4.769 seconds 6250000 operations;   28.1 MB/s (41001 of 250000 found)
After this PR:
readwhilewriting :      16.943 micros/op 1439376 ops/sec 4.342 seconds 6250000 operations;   23.8 MB/s (28977 of 250000 found)

add compaction range del aggregator to microbench
@cbi42 cbi42 changed the title Improve FragmentTombstone() speed by lazily initialize seq_set_ Improve FragmentTombstones() speed by lazily initialize seq_set_ Oct 24, 2022
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@cbi42 has updated the pull request. You must reimport the pull request before landing.

@cbi42 cbi42 changed the title Improve FragmentTombstones() speed by lazily initialize seq_set_ Improve FragmentTombstones() speed by lazily initializing seq_set_ Oct 24, 2022
@facebook-github-bot
Copy link
Contributor

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

1 similar comment
@facebook-github-bot
Copy link
Contributor

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

@cbi42 cbi42 requested a review from ajkr October 24, 2022 21:24
Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

Nice finding, LGTM!

@cbi42
Copy link
Member Author

cbi42 commented Oct 25, 2022

Nice finding, LGTM!

Thanks for the review!

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