Add RangeDelAggregator microbenchmarks #4363
Conversation
To measure the results of upcoming DeleteRange v2 work, this commit adds simple benchmarks for RangeDelAggregator. It measures the average time for AddTombstones and ShouldDelete calls.
There was a problem hiding this comment.
Nice! Great job finding all the right internal APIs to use.
Let's add "db/range_del_aggregator_bench.cc" to MAIN_SOURCES in "src.mk". That'll cause our Makefile to generate a "db/range_del_aggregator_bench.cc.d" file containing the header dependencies, which allows make to automatically rebuild your object file when those header changes. That avoids cases where we sometimes encounter confusing errors that require make clean && make ... to fix.
We can also consider adding the benchmark to BENCHMARKS in "CMakeLists.txt" for cmake build, and also to "TARGETS" file for internal buck build (sorry our build system is really fragmented :/).
As we discussed in person, a future PR could call AddTombstones multiple times per run. Our design for RangeDelAggregator behaves differently in that case, and that is the way RangeDelAggregator is used in real DBs.
db/range_del_aggregator_bench.cc
Outdated
| rocksdb::RangeDelAggregator range_del_agg(icmp, {} /* snapshots */, | ||
| FLAGS_use_collapsed); | ||
|
|
||
| for (int j = 0; j < FLAGS_num_range_tombstones; j++) { |
There was a problem hiding this comment.
Something to think about in the future. We might be getting artificially good cache perf by creating the tombstones right before they're added. While in practice, the range tombstone meta-block will often be cold when AddTombstones() is called.
There was a problem hiding this comment.
I think at least some of the range tombstones (primarily the ones in the memtable and L0) will be warm if the user issues lots of reads, because those range tombstones will be read for each of those calls. Also, even disregarding cache perf, I think the relative benchmark changes would still be useful to measure performance improvements, since the cache hit rate should be the same before and after the changes. Regardless, this probably requires some more thought, so I left a TODO to look at this more carefully later.
There was a problem hiding this comment.
abhimadan has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
abhimadan has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| @@ -919,6 +919,11 @@ ROCKS_TESTS = [ | |||
| "db/range_del_aggregator_test.cc", | |||
| "serial", | |||
| ], | |||
| [ | |||
| "range_del_aggregator_bench", | |||
There was a problem hiding this comment.
Since this is a benchmark, should we exclude it from the list of tests that will run in sandcastle?
There was a problem hiding this comment.
I'm removing it in #4426. Please let me know if I shouldn't do it. Thanks.
Summary: Update template of TARGETS file according to recent changes in facebook#4371 , facebook#4363 and facebook@dbf44c3. Pull Request resolved: facebook#4426 Differential Revision: D10025053 Pulled By: yiwu-arbug fbshipit-source-id: e6a0a702bfd401fc1af240ee446f5690f0bcd85d
To measure the results of upcoming DeleteRange v2 work, this commit adds
simple benchmarks for RangeDelAggregator. It measures the average time
for AddTombstones and ShouldDelete calls.
Using this to compare the results before #4014 and on the latest master (using the default arguments) produces the following results:
Before #4014:
Latest master: