Add bitmap control for perf context #237

LemonHX · 2021-06-08T02:20:27Z

design spec in on: https://docs.google.com/document/d/1JYmWMIZwYV0AZW6rNv_oFVZLBCAkxLLYslt39eEZstM/edit?usp=sharing Signed-off-by: lemonhx <[email protected]>

include/rocksdb/c.h

Connor1996

LGTM overall

db/c.cc

include/rocksdb/perf_flag_defs.h

monitoring/perf_flag.cc

breezewish · 2021-07-15T02:24:56Z

monitoring/perf_flag_test.cc

+        SetPerfLevel(PerfLevel::kDisable);
+        EnablePerfFlag(flag_user_key_comparison_count);
+      },
+      { ASSERT_GT(get_perf_context()->user_key_comparison_count, 0); });


Perf context is stored locally so that it may contain data from previous tests. I would suggest you to assert the delta value.

yiwu-arbug · 2021-07-29T02:52:10Z

/run-tests

yiwu-arbug · 2021-07-29T02:59:56Z

@LemonHX build fail. along with CMakeList.txt, you also need to add the new files and new tests to src.mk and https://github.com/tikv/rocksdb/blob/6.4.tikv/Makefile#L420

yiwu-arbug · 2021-07-29T03:23:13Z

/run-tests

LemonHX · 2021-08-02T00:48:40Z

@LemonHX build fail. along with CMakeList.txt, you also need to add the new files and new tests to src.mk and https://github.com/tikv/rocksdb/blob/6.4.tikv/Makefile#L420

done

LemonHX · 2021-08-02T00:48:53Z

/run-tests

yiwu-arbug · 2021-08-02T04:51:28Z

/run-tests

CMakeLists.txt

yiwu-arbug · 2021-08-03T18:41:26Z

The encryption_env test and Mac test in CI are broken. We can ignore them for now.

LemonHX · 2021-08-06T00:21:35Z

so could we merge it?

monitoring/perf_flag_test.cc

monitoring/perf_flag.cc

monitoring/perf_context_imp.h

include/rocksdb/perf_flag.h

CMakeLists.txt

db/c.cc

tabokie · 2021-08-06T16:49:26Z

@yiwu-arbug Are we running clang-format in CI right now? I wonder how these styling issues go unalerted.

tabokie · 2021-08-09T03:44:02Z

@LemonHX Could you run this line in your repo to format this PR manually? I'm not sure if there's any issues I haven't sighted.

git diff -U0 f95aaf8dac667334bb6314bd06d80749f63177bb | python <(curl https://raw.githubusercontent.com/llvm-mirror/clang/master/tools/clang-format/clang-format-diff.py) -p1 -i

And a few commit signoffs need fixing.

fix indentation error in `c.cc` adding comment on `FLAG_END` explain what does `& 0b111` does fix a huge mistake in `CheckPerfFlag` and writing the edge case tests for `perf_flag` Signed-off-by: lemonhx <[email protected]>

add test into `Makefile` and removed unused variable. fix indentation error in `CMakeLists` Signed-off-by: lemonhx <[email protected]>

Co-authored-by: Xinye Tao <[email protected]> Signed-off-by: lemonhx <[email protected]>

Signed-off-by: tabokie <[email protected]>

tabokie

I've manually resolved all the review comments. LGTM

Connor1996 · 2021-08-12T09:27:41Z

include/rocksdb/perf_flag.h

+
+#define FLAGS_LEN                               \
+  (((uint64_t)FLAG_END & (uint64_t)0b111) == 0  \
+       ? ((uint64_t)FLAG_END >> (uint64_t)0b11) \


Suggested change

? ((uint64_t)FLAG_END >> (uint64_t)0b11) \

? ((uint64_t)FLAG_END >> (uint64_t)3) \

Connor1996 · 2021-08-12T09:27:51Z

include/rocksdb/perf_flag.h

+#define FLAGS_LEN                               \
+  (((uint64_t)FLAG_END & (uint64_t)0b111) == 0  \
+       ? ((uint64_t)FLAG_END >> (uint64_t)0b11) \
+       : ((uint64_t)FLAG_END >> (uint64_t)0b11) + (uint64_t)1)


Suggested change

: ((uint64_t)FLAG_END >> (uint64_t)0b11) + (uint64_t)1)

: ((uint64_t)FLAG_END >> (uint64_t)3) + (uint64_t)1)

Signed-off-by: tabokie <[email protected]>

Connor1996

LGTM

This PR changes some bits of #237. Considering our use cases in TiKV, _although not really necessary_, we frequently set perf levels around RocksDB operations. So, if we switch to perf flags, it requires us to set perf flags efficiently. And we have quite a few flags to set, setting flags one by one in #237 is not that efficient. All flags can be saved in two words. So, we **can** achieve similar efficiency by setting all flags all at once. And meanwhile, we can flexibly specify which metrics we care about at runtime. This PR also changes to use `std::bitset` and a named enum instead of handmade int array and anonymous enum. Hopefully they will make the code cleaner. See tikv/rust-rocksdb#682 for the companion Rust wrapper Signed-off-by: Yilin Chen <[email protected]>

* `PerfFlag` implementation aside the `PerfLevel` design spec in on: https://docs.google.com/document/d/1JYmWMIZwYV0AZW6rNv_oFVZLBCAkxLLYslt39eEZstM/edit?usp=sharing Signed-off-by: lemonhx <[email protected]> Co-authored-by: Xinye Tao <[email protected]> Signed-off-by: tabokie <[email protected]>

This PR changes some bits of tikv#237. Considering our use cases in TiKV, _although not really necessary_, we frequently set perf levels around RocksDB operations. So, if we switch to perf flags, it requires us to set perf flags efficiently. And we have quite a few flags to set, setting flags one by one in tikv#237 is not that efficient. All flags can be saved in two words. So, we **can** achieve similar efficiency by setting all flags all at once. And meanwhile, we can flexibly specify which metrics we care about at runtime. This PR also changes to use `std::bitset` and a named enum instead of handmade int array and anonymous enum. Hopefully they will make the code cleaner. See tikv/rust-rocksdb#682 for the companion Rust wrapper Signed-off-by: Yilin Chen <[email protected]> Signed-off-by: tabokie <[email protected]>

* `PerfFlag` implementation aside the `PerfLevel` design spec in on: https://docs.google.com/document/d/1JYmWMIZwYV0AZW6rNv_oFVZLBCAkxLLYslt39eEZstM/edit?usp=sharing Signed-off-by: lemonhx <[email protected]> Co-authored-by: Xinye Tao <[email protected]> Signed-off-by: tabokie <[email protected]>

This PR changes some bits of #237. Considering our use cases in TiKV, _although not really necessary_, we frequently set perf levels around RocksDB operations. So, if we switch to perf flags, it requires us to set perf flags efficiently. And we have quite a few flags to set, setting flags one by one in #237 is not that efficient. All flags can be saved in two words. So, we **can** achieve similar efficiency by setting all flags all at once. And meanwhile, we can flexibly specify which metrics we care about at runtime. This PR also changes to use `std::bitset` and a named enum instead of handmade int array and anonymous enum. Hopefully they will make the code cleaner. See tikv/rust-rocksdb#682 for the companion Rust wrapper Signed-off-by: Yilin Chen <[email protected]> Signed-off-by: tabokie <[email protected]>

* `PerfFlag` implementation aside the `PerfLevel` design spec in on: https://docs.google.com/document/d/1JYmWMIZwYV0AZW6rNv_oFVZLBCAkxLLYslt39eEZstM/edit?usp=sharing Signed-off-by: lemonhx <[email protected]> Co-authored-by: Xinye Tao <[email protected]> Signed-off-by: tabokie <[email protected]>

This PR changes some bits of tikv#237. Considering our use cases in TiKV, _although not really necessary_, we frequently set perf levels around RocksDB operations. So, if we switch to perf flags, it requires us to set perf flags efficiently. And we have quite a few flags to set, setting flags one by one in tikv#237 is not that efficient. All flags can be saved in two words. So, we **can** achieve similar efficiency by setting all flags all at once. And meanwhile, we can flexibly specify which metrics we care about at runtime. This PR also changes to use `std::bitset` and a named enum instead of handmade int array and anonymous enum. Hopefully they will make the code cleaner. See tikv/rust-rocksdb#682 for the companion Rust wrapper Signed-off-by: Yilin Chen <[email protected]> Signed-off-by: tabokie <[email protected]>

PerfFlag implementation aside the PerfLevel

f955144

design spec in on: https://docs.google.com/document/d/1JYmWMIZwYV0AZW6rNv_oFVZLBCAkxLLYslt39eEZstM/edit?usp=sharing Signed-off-by: lemonhx <[email protected]>

LemonHX force-pushed the 6.4.tikv branch from 8405294 to f955144 Compare July 2, 2021 05:02

Connor1996 reviewed Jul 6, 2021

View reviewed changes

include/rocksdb/c.h Outdated Show resolved Hide resolved

Connor1996 reviewed Jul 6, 2021

View reviewed changes

breezewish reviewed Jul 6, 2021

View reviewed changes

db/c.cc Outdated Show resolved Hide resolved

include/rocksdb/perf_flag_defs.h Show resolved Hide resolved

breezewish reviewed Jul 10, 2021

View reviewed changes

monitoring/perf_flag.cc Show resolved Hide resolved

breezewish reviewed Jul 15, 2021

View reviewed changes

yiwu-arbug reviewed Aug 2, 2021

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

tabokie reviewed Aug 6, 2021

View reviewed changes

tabokie changed the title ~~Perf Flags~~ Add bitmap control for perf context Aug 6, 2021

tabokie reviewed Aug 6, 2021

View reviewed changes

db/c.cc Outdated Show resolved Hide resolved

LemonHX and others added 3 commits August 11, 2021 08:27

Adding some test and fixing git errors

f113d65

fix indentation error in `c.cc` adding comment on `FLAG_END` explain what does `& 0b111` does fix a huge mistake in `CheckPerfFlag` and writing the edge case tests for `perf_flag` Signed-off-by: lemonhx <[email protected]>

add source to src.mk

4ff539b

add test into `Makefile` and removed unused variable. fix indentation error in `CMakeLists` Signed-off-by: lemonhx <[email protected]>

fix formatting and styling issues.

c05a112

Co-authored-by: Xinye Tao <[email protected]> Signed-off-by: lemonhx <[email protected]>

LemonHX force-pushed the 6.4.tikv branch from d58579a to c05a112 Compare August 11, 2021 00:33

tabokie added 3 commits August 12, 2021 12:15

address comments

5a54760

Signed-off-by: tabokie <[email protected]>

remove redundant line

7aa7f27

Signed-off-by: tabokie <[email protected]>

trigger rebuild

8dbd097

Signed-off-by: tabokie <[email protected]>

tabokie approved these changes Aug 12, 2021

View reviewed changes

tabokie requested a review from Connor1996 August 12, 2021 06:28

Connor1996 reviewed Aug 12, 2021

View reviewed changes

address comments

9e34b86

Signed-off-by: tabokie <[email protected]>

tabokie requested a review from Connor1996 August 12, 2021 10:04

Connor1996 approved these changes Aug 13, 2021

View reviewed changes

tabokie merged commit 3de4cc2 into tikv:6.4.tikv Aug 13, 2021

ti-srebot mentioned this pull request Aug 13, 2021

rocksdb: Add bitmap control for perf context tikv/rust-rocksdb#651

Closed

sticnarf mentioned this pull request Apr 1, 2022

Set perf flags all at once #272

Merged

tabokie mentioned this pull request May 9, 2022

Upgrade to facebook 6.29 #277

Closed

39 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bitmap control for perf context #237

Add bitmap control for perf context #237

LemonHX commented Jun 8, 2021 •

edited

Loading

Connor1996 left a comment

breezewish Jul 15, 2021

yiwu-arbug commented Jul 29, 2021

yiwu-arbug commented Jul 29, 2021

yiwu-arbug commented Jul 29, 2021

LemonHX commented Aug 2, 2021

LemonHX commented Aug 2, 2021

yiwu-arbug commented Aug 2, 2021

yiwu-arbug commented Aug 3, 2021

LemonHX commented Aug 6, 2021

tabokie commented Aug 6, 2021

tabokie commented Aug 9, 2021

tabokie left a comment

Connor1996 Aug 12, 2021

Connor1996 Aug 12, 2021

Connor1996 left a comment

	? ((uint64_t)FLAG_END >> (uint64_t)0b11) \
	? ((uint64_t)FLAG_END >> (uint64_t)3) \

	: ((uint64_t)FLAG_END >> (uint64_t)0b11) + (uint64_t)1)
	: ((uint64_t)FLAG_END >> (uint64_t)3) + (uint64_t)1)

Add bitmap control for perf context #237

Add bitmap control for perf context #237

Conversation

LemonHX commented Jun 8, 2021 • edited Loading

Connor1996 left a comment

Choose a reason for hiding this comment

breezewish Jul 15, 2021

Choose a reason for hiding this comment

yiwu-arbug commented Jul 29, 2021

yiwu-arbug commented Jul 29, 2021

yiwu-arbug commented Jul 29, 2021

LemonHX commented Aug 2, 2021

LemonHX commented Aug 2, 2021

yiwu-arbug commented Aug 2, 2021

yiwu-arbug commented Aug 3, 2021

LemonHX commented Aug 6, 2021

tabokie commented Aug 6, 2021

tabokie commented Aug 9, 2021

tabokie left a comment

Choose a reason for hiding this comment

Connor1996 Aug 12, 2021

Choose a reason for hiding this comment

Connor1996 Aug 12, 2021

Choose a reason for hiding this comment

Connor1996 left a comment

Choose a reason for hiding this comment

LemonHX commented Jun 8, 2021 •

edited

Loading