Skip to content
New issue

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

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

Already on GitHub? # to your account

Avoid cache lookups for range deletion meta-block #1801

Closed
wants to merge 2 commits into from

Conversation

ajkr
Copy link
Contributor

@ajkr ajkr commented Jan 24, 2017

I added the Cache::Ref() function a couple weeks ago (#1761) to make this feature possible. Like other meta-blocks, rep_->range_del_entry holds a cache handle to pin the range deletion block in uncompressed block cache for the duration of the table reader's lifetime. We can reuse this cache handle to create an iterator over this meta-block without any cache lookup. Ref() is used to increment the cache handle's refcount in case the returned iterator outlives the table reader.

Test Plan:

Measurements:

  • random read perf before this diff: 12.0 MB/s
  • random read perf after this diff: 13.4 MB/s
  • baseline random read perf (using -expand_range_tombstones): 15-16 MB/s

Commands:

  • fill db:
    ./db_bench -benchmarks=fillrandom -write_buffer_size=67108864 -level0_file_num_compaction_trigger=4 -max_background_compactions=32 -level0_slowdown_writes_trigger=8 -level0_stop_writes_trigger=12 -num_levels=4 -min_level_to_compress=2 -compression_type=zlib -compression_ratio=0.5 -block_size=16384 -open_files=10000 -target_file_size_base=67108864 -max_bytes_for_level_base=268435456 -writes_per_range_tombstone=50000 -range_tombstone_width=50000 -max_num_range_tombstones=1000 -num=50000000
  • random read:
    ./db_bench -benchmarks=readrandom -readonly -use_existing_db=true -open_files=10000 -num=50000000 -reads=1000000 -cache_size=1073741824 -threads=32

@facebook-github-bot
Copy link
Contributor

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

@ajkr ajkr requested review from siying and yiwu-arbug January 25, 2017 17:58
@yiwu-arbug
Copy link
Contributor

What's the reason to do Ref() only instead of normal lookup?

@ajkr
Copy link
Contributor Author

ajkr commented Jan 25, 2017

Oh, Lookup() was consuming a lot of CPU (~5% in my profiling) for computing hash to lookup in handle table. But it's not necessary when we already have a handle.

Copy link
Contributor

@siying siying left a comment

Choose a reason for hiding this comment

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

I'm surprised how complicated it is now.

@ajkr
Copy link
Contributor Author

ajkr commented Jan 25, 2017

maybe i've been desensitized to overengineering, but its complexity is pretty in line with my expectations.

@ajkr ajkr force-pushed the del-range-reuse-cache-handle branch from 63755c2 to 56b2629 Compare January 26, 2017 04:01
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

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

Successfully merging this pull request may close these issues.

4 participants