-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
Add last level and non-last level read statistics #9519
Conversation
@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
file/random_access_file_reader.cc
Outdated
@@ -276,6 +276,13 @@ IOStatus RandomAccessFileReader::Read(const IOOptions& opts, uint64_t offset, | |||
IOStatsAddCountByTemperature(file_temperature_, 1); | |||
StatisticAddBytesByTemperature(stats_, file_temperature_, result->size()); | |||
StatisticAddCountByTemperature(stats_, file_temperature_, 1); | |||
if (is_last_level_) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this if. This will create branch misprediction in a critical path. Although this is unlikely to be the bottleneck but small things might add up. Is it possible to remember the two tick names when creating RandomAccessFileReader?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually the same as the temperature checks above. I'll see if we can optimize them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated that part a little bit, basically grouping the temperature checking, which reduced the temperature if checking 3 times even when it's not enabled. I added a microbenchmark test, which shows the performance is even improved when statistics not enabled:
Main: 255ns
With fix: 252ns
Old fix: 256ns
When statistics is enabled, the time is increased because more statistics recording, which is unavoidable for more statistics:
Main: 340ns
With fix: 370ns
Old fix: 374ns
Benchmark command:
$ DEBUG_LEVEL=0 make db_basic_bench
$ ./db_basic_bench --benchmark_filter=RandomAccessFileReaderRead --benchmark_repetitions=50
bac3887
to
b09e1e1
Compare
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
b09e1e1
to
b3201f9
Compare
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
1 similar comment
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
file/random_access_file_reader.cc
Outdated
RecordTick(stats, LAST_LEVEL_READ_COUNT, 1); | ||
} else { | ||
RecordTick(stats, NON_LAST_LEVEL_READ_BYTES, size); | ||
RecordTick(stats, NON_LAST_LEVEL_READ_COUNT, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think it is more commonly recoreded compared to temperature, so it might make sense to move it up.
file/random_access_file_reader.cc
Outdated
if (stats == nullptr || file_temperature == Temperature::kUnknown) { | ||
return; | ||
inline void RecordStats(Statistics* stats, Temperature file_temperature, | ||
bool is_last_level, size_t size) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps call it something like RecordIOStats() or something like that so that size
can be more obvious to be read size from the file.
53797c8
to
bea844f
Compare
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
1 similar comment
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
8a9e6a9
to
58da985
Compare
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: Add last level and non-last level read statistics:
Test Plan: added unittest