use per-level perfcontext for DB::Get calls #4617
Conversation
db/version_set.cc
Outdated
| @@ -1218,6 +1218,7 @@ void Version::Get(const ReadOptions& read_options, const LookupKey& k, | |||
| sample_file_read_inc(f->file_metadata); | |||
| } | |||
|
|
|||
| StopWatchNano timer(env_, true); | |||
There was a problem hiding this comment.
We should only do timing when perf context level allows timing and by level counter is enabled.
db/db_impl_readonly.cc
Outdated
| @@ -52,11 +52,17 @@ Status DBImplReadOnly::Get(const ReadOptions& read_options, | |||
| &max_covering_tombstone_seq, read_options)) { | |||
| pinnable_val->PinSelf(); | |||
| RecordTick(stats_, MEMTABLE_HIT); | |||
| PERF_COUNTER_BY_LEVEL_ADD(user_key_return_count, 1, 0); | |||
There was a problem hiding this comment.
Memtable is not level0. Now we can't distinguish the two.
db/db_impl_readonly.cc
Outdated
| super_version->current->Get(read_options, lkey, pinnable_val, &s, | ||
| &merge_context, &max_covering_tombstone_seq); | ||
| &merge_context, &level_found, |
There was a problem hiding this comment.
Version::Get() to return level found makes the function less clean. Do we really need to return it, or can be update the counter inside Version::Get()?
There was a problem hiding this comment.
Sure, we can update the counter inside Version::Get(), doesn't have to happen in DBImpl::GetImpl
There was a problem hiding this comment.
@miasantreble has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
db/version_set.cc
Outdated
| @@ -1218,6 +1218,9 @@ void Version::Get(const ReadOptions& read_options, const LookupKey& k, | |||
| sample_file_read_inc(f->file_metadata); | |||
| } | |||
|
|
|||
| bool timer_enabled = get_perf_context()->per_level_perf_context_enabled && | |||
| GetPerfLevel() >= PerfLevel::kEnableTimeExceptForMutex; | |||
There was a problem hiding this comment.
Reverting the two conditions feels slightly more performant in normal path to me.
| @@ -1250,6 +1257,7 @@ void Version::Get(const ReadOptions& read_options, const LookupKey& k, | |||
| } else if (fp.GetHitFileLevel() >= 2) { | |||
| RecordTick(db_statistics_, GET_HIT_L2_AND_UP); | |||
| } | |||
| PERF_COUNTER_BY_LEVEL_ADD(user_key_return_count, 1, fp.GetHitFileLevel()); | |||
There was a problem hiding this comment.
I think we need to find it in kMerge too.
There was a problem hiding this comment.
I think we don't deal with all kMerge case now. It's OK for now if you add a TODO here, and comment in the perf counter what the expectation is.
include/rocksdb/perf_context.h
Outdated
| @@ -26,6 +28,10 @@ struct PerfContextByLevel { | |||
| // exist. | |||
| uint64_t bloom_filter_full_true_positive = 0; | |||
|
|
|||
| uint64_t user_key_return_count; // total number of user key returned | |||
There was a problem hiding this comment.
Need to have better comment. For example, explaining how it counts in deletion and merge cases.
include/rocksdb/perf_context.h
Outdated
| @@ -26,6 +28,10 @@ struct PerfContextByLevel { | |||
| // exist. | |||
| uint64_t bloom_filter_full_true_positive = 0; | |||
|
|
|||
| uint64_t user_key_return_count; // total number of user key returned | |||
|
|
|||
| uint64_t get_processing_time; // total nanos spent on Get query | |||
There was a problem hiding this comment.
We used to do ..._time, but later realized that it was not good. Now the convention is ..._nanos. This can be something like get_from_table_nanos.
|
@miasantreble has updated the pull request. Re-import the pull request |
|
all comments have been addressed, thanks @siying |
There was a problem hiding this comment.
@miasantreble has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
include/rocksdb/perf_context.h
Outdated
| @@ -26,6 +28,12 @@ struct PerfContextByLevel { | |||
| // exist. | |||
| uint64_t bloom_filter_full_true_positive = 0; | |||
|
|
|||
| // total number of user key returned (only include keys that are found, does | |||
| // not include keys that are deleted or merged | |||
There was a problem hiding this comment.
This can be more accurate. If the key is found as "merge-merge-put", we still report one. But if there is no final put, we don't report. It's a good idea to clarify.
include/rocksdb/perf_context.h
Outdated
| // not include keys that are deleted or merged | ||
| uint64_t user_key_return_count; | ||
|
|
||
| uint64_t get_from_table_nanos; // total nanos spent on Get query |
There was a problem hiding this comment.
Not "get" query, but getting from SST files. Memtable and other operations are not included here.
|
Also HISTORY.md. |
|
@miasantreble has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@miasantreble is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
this PR adds two more per-level perf context counters to track