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

Sample number of reads per SST file #2417

Closed
wants to merge 1 commit into from

Conversation

siying
Copy link
Contributor

@siying siying commented Jun 6, 2017

Summary:We estimate number of reads per SST files, by updating the counter per file in sampled read requests. This information can later be used to trigger compactions to improve read performacne.

Test Plan:
Load the DB with
./db_bench --benchmarks=fillrandom,sstables --write_buffer_size=2000000 --num=3000000

And then observe stats in outputs of:

./db_bench --benchmarks=readrandom,sstables --write_buffer_size=2000000 --num=3000000 --threads=8

and

./db_bench --benchmarks=seekrandom,sstables --write_buffer_size=2000000 --num=3000000 --threads=8

@facebook-github-bot
Copy link
Contributor

@siying updated the pull request - view changes

@facebook-github-bot
Copy link
Contributor

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

Copy link
Contributor

@sagar0 sagar0 left a comment

Choose a reason for hiding this comment

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

a few nits. Looks good to me otherwise.

: icmp_(icmp),
flevel_(flevel),
index_(static_cast<uint32_t>(flevel->num_files)),
current_value_(0, 0, 0) { // Marks as invalid
current_value_(0, 0, 0),
should_sample_(should_sample) { // Marks as invalid
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment "Marks as invalid" should be against current_value_ (i.e. the FileDescriptor instance).

@@ -122,7 +121,7 @@ class FilePicker {
}
}

int GetCurrentLevel() { return returned_file_level_; }
int curr_level() const { return curr_level_; }
Copy link
Contributor

Choose a reason for hiding this comment

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

The function name should still be GetCurrentLevel following the convention of other function names in this class, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I'll revert it to avoid confusion. In the next TechDebt Week, maybe we should clear up these functions to follow the convention mentioned in Google C++ Style Guide: https://google.github.io/styleguide/cppguide.html#Function_Names . Now we have both cases in the code base.

#include "util/random.h"

namespace rocksdb {
static const uint32_t kFileReadSampleRate = 1024;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering out loud, if there are any cases in which we want to make the sampling rate configurable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea. We can make it configurable once there is such a case. So far I would keep it hard-coded to avoid yet another option.

Summary:We estimate number of reads per SST files, by updating the counter per file in sampled read requests. This information can later be used to trigger compactions to improve read performacne.

Test Plan:
Load the DB with
./db_bench --benchmarks=fillrandom,sstables --write_buffer_size=2000000 --num=3000000

And then observe stats in outputs of:

./db_bench --benchmarks=readrandom,sstables --write_buffer_size=2000000 --num=3000000 --threads=8

and

./db_bench --benchmarks=seekrandom,sstables --write_buffer_size=2000000 --num=3000000 --threads=8
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@siying 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.

3 participants