Add more micro-benchmark tests #9436

jay-zhuang · 2022-01-25T21:12:27Z

Add more micro-benchmark tests
Expose an API in DBImpl for waiting for compactions (still not visible to the user)
Add argument name for ribbon_bench
remove benchmark run from CI, as it runs too long.

Test: CI

facebook-github-bot · 2022-01-25T22:39:53Z

@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-01-25T22:45:10Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-01-25T22:45:33Z

@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-01-26T02:51:41Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

mdcallag · 2022-01-27T19:32:26Z

db/db_impl/db_impl_compaction_flush.cc

+  }
+  return error_handler_.GetBGError();
+}
+


Can the thing I added to db_bench be switched to using this? Not asking you to do that in this diff.
https://github.com/facebook/rocksdb/blob/main/tools/db_bench_tool.cc#L7788

Also, does this work for leveled, universal and integrated BlobDB? The thing I added to db_bench doesn't work for universal, see #9275

Can the thing I added to db_bench be switched to using this?

This is still an internal API which was used for unittest. To be used for db_bench, we need to expose this as a public API, which at least needs more polish and other considerations.

@mdcallag looking at the code, I think you can do dbimpl = static_cast<DBImpl*>(db_.db), but as @jay-zhuang pointed out, this is an internal function and may subject to change in the future.

db/db_impl/db_impl.h

riversand963

Some more comments.

riversand963 · 2022-01-27T20:05:54Z

microbench/db_basic_bench.cc

-// this is a simple micro-benchmark for compare ribbon filter vs. other filter
-// for more comprehensive, please check the dedicate util/filter_bench.
-#include <benchmark/benchmark.h>
+#include <unistd.h>


May be a good idea to exclude Windows from compilation.

riversand963 · 2022-01-28T18:10:15Z

microbench/db_basic_bench.cc

+  DB* db;
+  Options options;
+  SetupDB(state, options, &db, "DBOpen");
+


May be a good idea to init to nullptr.

riversand963 · 2022-01-28T18:11:22Z

microbench/db_basic_bench.cc

+  uint64_t key_num = max_data / per_key_size;
+
+  // setup DB
+  static DB* db;


Is it the intention to make db a static variable?

Yes, it's used for multi-threaded test, the db is shared between threads: https://github.com/google/benchmark/blob/main/docs/user_guide.md#multithreaded-benchmarks

riversand963 · 2022-01-28T18:17:07Z

microbench/db_basic_bench.cc

+
+  if (state.thread_index == 0) {
+    auto db_full = static_cast_with_check<DBImpl>(db);
+    auto s = db_full->WaitForCompact(true);


This may be minor. According to https://google.github.io/styleguide/cppguide.html#Type_deduction which we follow, we usually use auto when it's clear what the type is. The static_cast cases fall into this category. For others, if the type is not that clear, we should still explicitly spell out the type.

riversand963 · 2022-01-28T18:20:10Z

microbench/db_basic_bench.cc

+               "enable_filter"});
+}
+
+static const uint64_t PrefixSeekNum = 10l << 10;


Nit: constexpr may be better, but this is minor. The name should be kPrefixSeekNum.

riversand963 · 2022-01-28T18:22:02Z

microbench/db_basic_bench.cc

+  BlockContents contents;
+  contents.data = rawblock;
+  Block reader(std::move(contents));
+  const InternalKeyComparator icmp(BytewiseComparator());


Is BytesiseComparator() intended to be hard-coded?

good point, removed that and use options.comparator instead.

riversand963 · 2022-01-31T17:23:55Z

Also, building on non-fb machine will fail because the dependency on google benchmark is hardcoded. It will be nice to make it work for the following

ROCKSDB_NO_FBCODE=1 make -j20 db_basic_bench

jay-zhuang · 2022-01-31T19:51:23Z

Also, building on non-fb machine will fail because the dependency on google benchmark is hardcoded. It will be nice to make it work for the following
ROCKSDB_NO_FBCODE=1 make -j20 db_basic_bench

it should work, can you try install benchmark:

sudo dnf install google-benchmark-devel
make clean && ROCKSDB_NO_FBCODE=1 make -j20 db_basic_bench

riversand963 · 2022-01-31T19:55:28Z

Sorry for the confusion. I meant "making the detection of benchmark-devel package more automatic", like what we do for some other packages, e.g. gflags, etc. We can detect the package in CMakeLists.txt and set some variables. In the .cc file, we put #ifdef GOOGLE_BENCHMARK around the code that rely on it.
Not that important, I do not have a strong opinion on this.

facebook-github-bot · 2022-01-31T21:53:38Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

which takes too long

facebook-github-bot · 2022-02-01T00:25:55Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-02-01T00:26:57Z

@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

riversand963

Didn't review in too much detail. LGTM since it's microbenchmark and in active development phase.

riversand963 · 2022-02-01T00:37:02Z

microbench/db_basic_bench.cc

+  options.level0_stop_writes_trigger = (1 << 30);
+  options.soft_pending_compaction_bytes_limit = 0;
+  options.hard_pending_compaction_bytes_limit = 0;
+  options.write_buffer_size = 2 << 30;  // 2G to avoid auto flush


Maybe a good idea to cast 2 first before shifting.

sorry, do you mean like:

options.write_buffer_size = 2l << 30;

2ull << 30 or static_cast<size_t>(2) << 30

jay-zhuang · 2022-02-01T00:44:28Z

Sorry for the confusion. I meant "making the detection of benchmark-devel package more automatic", like what we do for some other packages, e.g. gflags, etc. We can detect the package in CMakeLists.txt and set some variables. In the .cc file, we put #ifdef GOOGLE_BENCHMARK around the code that rely on it. Not that important, I do not have a strong opinion on this.

I see, I think microbenchmark might be different. It is required to build all mircobench tests: https://github.com/facebook/rocksdb/blob/main/microbench/CMakeLists.txt#L1
If the user don't want to build it or don't have gbenchmark installed, they should not try build it. By default, it's disabled: https://github.com/facebook/rocksdb/blob/main/CMakeLists.txt#L1463

facebook-github-bot · 2022-02-01T00:51:52Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-02-01T04:03:48Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-02-01T04:04:25Z

@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot added the CLA Signed label Jan 25, 2022

jay-zhuang force-pushed the bench_test branch from 42cf782 to dc9eead Compare January 25, 2022 21:12

jay-zhuang marked this pull request as ready for review January 25, 2022 22:39

mdcallag reviewed Jan 27, 2022

View reviewed changes

riversand963 reviewed Jan 27, 2022

View reviewed changes

db/db_impl/db_impl.h Show resolved Hide resolved

riversand963 reviewed Jan 28, 2022

View reviewed changes

jay-zhuang force-pushed the bench_test branch from 693c0e2 to a65c251 Compare January 31, 2022 21:53

jay-zhuang added 5 commits January 31, 2022 16:25

Add more micro-benchmark tests

2924819

Fix build

94ab4be

don't run microbench in test

405665b

which takes too long

review feedbacks and clang-tidy

fa0bb11

more fixes

375b236

jay-zhuang force-pushed the bench_test branch from a65c251 to 375b236 Compare February 1, 2022 00:25

riversand963 approved these changes Feb 1, 2022

View reviewed changes

minor fix

28ae4a0

Fix iterator multi-thread issue

7a99cd4

facebook-github-bot closed this in 980b9ff Feb 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more micro-benchmark tests #9436

Add more micro-benchmark tests #9436

jay-zhuang commented Jan 25, 2022 •

edited

Loading

facebook-github-bot commented Jan 25, 2022

facebook-github-bot commented Jan 25, 2022

facebook-github-bot commented Jan 25, 2022

facebook-github-bot commented Jan 26, 2022

mdcallag Jan 27, 2022

jay-zhuang Jan 27, 2022

riversand963 Jan 31, 2022

riversand963 left a comment

riversand963 Jan 27, 2022

riversand963 Jan 28, 2022

riversand963 Jan 28, 2022

jay-zhuang Jan 31, 2022

riversand963 Jan 28, 2022

riversand963 Jan 28, 2022

riversand963 Jan 28, 2022

jay-zhuang Jan 31, 2022

riversand963 commented Jan 31, 2022

jay-zhuang commented Jan 31, 2022

riversand963 commented Jan 31, 2022

facebook-github-bot commented Jan 31, 2022

facebook-github-bot commented Feb 1, 2022

facebook-github-bot commented Feb 1, 2022

riversand963 left a comment

riversand963 Feb 1, 2022

jay-zhuang Feb 1, 2022

riversand963 Feb 1, 2022

jay-zhuang commented Feb 1, 2022

facebook-github-bot commented Feb 1, 2022

facebook-github-bot commented Feb 1, 2022

facebook-github-bot commented Feb 1, 2022

Add more micro-benchmark tests #9436

Add more micro-benchmark tests #9436

Conversation

jay-zhuang commented Jan 25, 2022 • edited Loading

facebook-github-bot commented Jan 25, 2022

facebook-github-bot commented Jan 25, 2022

facebook-github-bot commented Jan 25, 2022

facebook-github-bot commented Jan 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riversand963 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riversand963 commented Jan 31, 2022

jay-zhuang commented Jan 31, 2022

riversand963 commented Jan 31, 2022

facebook-github-bot commented Jan 31, 2022

facebook-github-bot commented Feb 1, 2022

facebook-github-bot commented Feb 1, 2022

riversand963 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jay-zhuang commented Feb 1, 2022

facebook-github-bot commented Feb 1, 2022

facebook-github-bot commented Feb 1, 2022

facebook-github-bot commented Feb 1, 2022

jay-zhuang commented Jan 25, 2022 •

edited

Loading