Perf Level By Bit Field #230
Conversation
…hods to perf_bit_field, also adding three default value for compatibility.
monitoring/perf_context_imp.h
Outdated
| } \ | ||
| } \ | ||
| // TODO: 添加一个flag count | ||
| #define PERF_COUNTER_BY_LEVEL_ADD(metric, value, level) \ |
There was a problem hiding this comment.
Do we need to change this part?
monitoring/perf_context_imp.h
Outdated
| #define PERF_TIMER_STOP(metric) | ||
| #define PERF_TIMER_START(metric) | ||
| #define PERF_TIMER_GUARD(metric) | ||
| #define PERF_TIMER_GUARD_WITH_ENV(metric, env) |
There was a problem hiding this comment.
It's interesting that the original one does not have this definition.. Is it a mistake in RocksDB?
include/rocksdb/perf_flags.h
Outdated
| uint8_t enable_key_lock_wait_count_bit : 1; // 2 | ||
|
|
||
| // flag enable for using cpu time prob useless in TiKV port | ||
| uint8_t enable_measure_cpu_time_bit : 1; // 3 -> with CPU time flag |
monitoring/perf_context_imp.h
Outdated
| } \ | ||
| // TODO: add a flag count | ||
| #define PERF_COUNTER_BY_LEVEL_ADD(metric, value, level) \ | ||
| if (perf_context.per_level_perf_context_enabled && \ |
There was a problem hiding this comment.
Seems that there is an optimization in the original implementation that it checks whether the perf is enabled. You should check it as well I think, considering that checking perf flags should be faster than the xxx.find() below.
also removed field .perf_level in PerfFlags
…e same time, it has the ability to adjust using BitMask, speeds up initialization, reduces unnecessary macros, and is compatible with PerfLevel's Get and Set. PerfLevel Get will now return an estimated PerfLevel instead of an accurate one.
| rocksdb_perfcontext_t* context = new rocksdb_perfcontext_t; | ||
| auto* context = new rocksdb_perfcontext_t; |
There was a problem hiding this comment.
I suspect whether this will cause issues in some build platforms.
| namespace rocksdb { | ||
| struct PerfFlags { | ||
| // represent original Level 2 | ||
| // TODO: think a better design for context_by_level structure |
| }; | ||
|
|
||
| // set the perf stats level for current thread | ||
| void SetPerfLevel(PerfLevel level); | ||
|
|
||
| // get current perf stats level for current thread | ||
| // get current estimated perf stats level for current thread |
| .enable_internal_merge_count_bit = 1, \ | ||
| .enable_get_from_memtable_count_bit = 1, \ | ||
| .enable_seek_on_memtable_count_bit = 1, \ | ||
| .enable_next_on_memtable_count_bit = 1, \ | ||
| .enable_prev_on_memtable_count_bit = 1, \ | ||
| .enable_seek_child_seek_count_bit = 1, \ | ||
| .enable_bloom_memtable_hit_count_bit = 1, \ | ||
| .enable_bloom_memtable_miss_count_bit = 1, \ | ||
| .enable_bloom_sst_hit_count_bit = 1, .enable_bloom_sst_miss_count_bit = 1, \ | ||
| .enable_key_lock_wait_count_bit = 1 |
| perf_level = level; | ||
| switch (level) { | ||
| case kEnableTime: | ||
| perf_flags = PERF_LEVEL5; |
There was a problem hiding this comment.
How about defining an array for these constants?
| PerfLevel GetPerfLevel() { | ||
| return perf_level; | ||
| void* levels[5] = {(void*)&PERF_LEVEL1, (void*)&PERF_LEVEL2, |
There was a problem hiding this comment.
I guess you can define PERF_LEVEL as a constant array, then you don't need to build the array here.
|
|
||
| #include <string> | ||
| #include <vector> | ||
|
|
There was a problem hiding this comment.
I suggest to change as less as possible. Otherwise it will be pain to merge upstream.
| #include "memory/arena.h" | ||
| #include "monitoring/histogram.h" | ||
| #include "monitoring/perf_context_imp.h" |
There was a problem hiding this comment.
Ditto. I suggest to only change necessary part, in order to reduce merge upstream effort.
Created Structure PerfLevelByBitField and using that structure to control the perf level tracing instead of PerfLevel, which allows us to more accurately control the behavior recorded by perf level.
And redirect set perf level to that for compatibility reason.
related issue No.543 in rust-rocksdb