Skip to content
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 micro-benchmark support #8493

Closed
wants to merge 2 commits into from

Conversation

jay-zhuang
Copy link
Contributor

@jay-zhuang jay-zhuang commented Jul 7, 2021

Summary: Add google benchmark for microbench.
Add ribbon_bench for benchmark ribbon filter vs. other filters.

Test Plan: added test to CI

@jay-zhuang jay-zhuang force-pushed the microbench_test2 branch 2 times, most recently from 0f63e26 to 6351857 Compare July 7, 2021 16:18
@jay-zhuang jay-zhuang changed the title [WIP] Add micro-benchmark Add micro-benchmark support Jul 7, 2021
@jay-zhuang jay-zhuang marked this pull request as ready for review July 7, 2021 16:19
@facebook-github-bot
Copy link
Contributor

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

Copy link
Contributor

@pdillinger pdillinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine, with some questions / critiques

@@ -370,6 +378,16 @@ jobs:
- run: CC=gcc-10 CXX=g++-10 V=1 SKIP_LINK=1 ROCKSDB_CXX_STANDARD=c++20 make -j16 all | .circleci/cat_ignore_eagain # Linking broken because libgflags compiled with newer ABI
- post-steps

build-linux-microbench:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you're intending CircleCI to function here as a smoke test for building & running the benchmarks, not for gathering high quality data (because running on a VM)?

Copy link
Contributor Author

@jay-zhuang jay-zhuang Jul 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, just to make sure the tests pass. added comment.

// (found in the LICENSE.Apache file in the root directory).

#include <benchmark/benchmark.h>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a reasonable starting point, but filter_bench is more representative of how filters are used in RocksDB, e.g. by queries hitting different filters occupying a large memory space. We should express here that the goal is to eventually get those kinds of results into the reusable benchmarking framework. One could get the impression that this file is exclusively focused on "more micro" benchmarking of filter behavior than filter_bench does, but I do not believe that is the goal. Others could assume this file is "authoritative" and no other benchmarking for filters exists.

In broader terms, I fear that even using the term "microbenchmark" might distract from the goal (as I see it) of taking meaningful measurements of subsystem performance to catch regressions (and track improvements) by focusing too much on individual components (e.g. querying a filter) rather than treating them as subsystems (e.g. querying one of many filters).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree both.

  1. this file is just the first benchmark that more for demonstrating the integration. Compare to filter_bench, it's missing lots tests, but I guess if we were having this microbenchmark framework before, we might just use it instead of creating stand-alone filter_bench. I'll add comment to refer to filter_bench for better filter benchmark.
  2. For microbenchmark focusing too much on small component, yes, I think it should not replace db_bench, it just gives extra, targeted information. it's also up to the test designer, so when writing future benchmark tests, we should have a more meaningful benchmark and metrics. Some metrics might be useful during development (like this one) but maybe not useful (or likely change) after feature is done, which we could exclude in the run or just ignore the result.

Summary: Add google benchmark for microbench.
Add ribbon_bench for benchmark ribbon filter vs. other filters.

Test Plan: added test to CI
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@jay-zhuang merged this pull request in 5dd18a8.

@mrambacher
Copy link
Contributor

This change will now generate build errors/warnings on any platform/host that does not have the benchmark installed:

make dbg
$DEBUG_LEVEL is 2
Makefile:176: Warning: Compiling in debug mode. Don't use the resulting binary in production
microbench/ribbon_bench.cc:8:10: fatal error: 'benchmark/benchmark.h' file not
found
#include <benchmark/benchmark.h>
^~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

I believe this is part of the dependency generation step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants