Refactor FilterPolicies toward Customizable #9567

pdillinger · 2022-02-14T23:16:44Z

Summary: Some changes to make it easier to make FilterPolicy
customizable. Especially, create distinct classes for the different
testing-only and user-facing built-in FilterPolicy modes.

Test Plan: tests updated, with no intended difference in functionality
tested. No difference in test performance seen as a result of moving to
string-based filter type configuration.

Summary: Some changes to make it easier to make FilterPolicy customizable. Especially, create distinct classes for the different testing-only and user-facing built-in FilterPolicy modes. Test Plan: tests updated, with no intended difference in functionality tested. No difference in test performance seen as a result of moving to string-based filter type configuration.

facebook-github-bot · 2022-02-15T04:23:21Z

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

mrambacher · 2022-02-15T12:27:41Z

table/block_based/filter_policy_internal.h

+  virtual std::string ToString() const = 0;
+


For consistency with Customizable, please rename this GetId(). Same signature

mrambacher · 2022-02-15T12:38:42Z

table/block_based/filter_policy_internal.h


-  inline int TEST_GetStartingLevelForB() const { return starting_level_for_b_; }
+  static constexpr const char* kName = "bloomfilter";


FWIW, the standard I have used for other Customizable classes is a function, not constexpr (static const char* kName() { return "bloomfilter"; }

Also, for Customizable, this becomes the "public" name of the class. So you will be creating "bloomfilter:4" for example.

Also, for Customizable, this becomes the "public" name of the class. So you will be creating "bloomfilter:4" for example.

Putting something like "bloomfilter:4:false" into the options file is intentional for forward compatibility. I don't think it's a good idea to deviate from that.

mrambacher · 2022-02-15T12:45:52Z

table/block_based/filter_policy.cc

+std::string BloomFilterPolicy::ToString() const {
+  // Including ":false" for better forward-compatibility
+  return kName + GetBitsPerKeySuffix() + ":false";
+}


Can you tell me what the ":false" is for?

See https://github.com/facebook/rocksdb/blob/6.29.fb/table/block_based/filter_policy.cc#L1582

Do you need the ":false" for compatibility though? Reading the code in options test, it accepts "bloomfilter:4" as well as "bloomfilter:4:false". See https://github.com/facebook/rocksdb/blob/main/options/options_test.cc#L934

You have linked to 'main'. I have linked to '6.29.fb'. Here's the test in 6.29.fb: https://github.com/facebook/rocksdb/blob/6.29.fb/options/options_test.cc#L931

table/block_based/filter_policy.cc

table/block_based/block_based_filter_block.cc

…zable

facebook-github-bot · 2022-02-15T16:53:59Z

@pdillinger has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-02-15T17:01:01Z

@pdillinger has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-02-15T17:37:24Z

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

db/db_bloom_filter_test.cc

util/filter_bench.cc

facebook-github-bot · 2022-02-15T21:57:59Z

@pdillinger has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-02-15T22:46:36Z

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

mrambacher

Thanks for the changes. LGTM.

mrambacher · 2022-02-16T00:48:07Z

table/block_based/filter_policy_internal.h

+      const FilterBuildingContext&) const override;
+  static constexpr size_t kSecretBitsPerKeyStart = 1234567890U;
+
+  static const char* kName();


Is there a reason not to put the implementation here as well? static const char* kName() { return "x.y.z"; }

Functions defined in headers slow down compilation and linking, so I avoid them when not performance critical or templated. Is there a good reason for making these functions as requested or just "consistency"?

This reverts commit 8c68108. The reason is the crash test began failing on the below assertion following this commit. ``` db_stress: table/block_based/filter_policy.cc:316: rocksdb::{anonymous}::FastLocalBloomBitsBuilder::FastLocalBloomBitsBuilder(int, std::atomic<long int>*, std::shared_ptr<rocksdb::CacheReservationManager>, bool): Assertion `millibits_per_key >= 1000' failed. ```

Summary: Users can set the priority for file reads associated with their operation by setting `ReadOptions::rate_limiter_priority` to something other than `Env::IO_TOTAL`. Rate limiting `VerifyChecksum()` and `VerifyFileChecksums()` is the motivation for this PR, so it also includes benchmarks and minor bug fixes to get that working. `RandomAccessFileReader::Read()` already had support for rate limiting compaction reads. I changed that rate limiting to be non-specific to compaction, but rather performed according to the passed in `Env::IOPriority`. Now the compaction read rate limiting is supported by setting `rate_limiter_priority = Env::IO_LOW` on its `ReadOptions`. There is no default value for the new `Env::IOPriority` parameter to `RandomAccessFileReader::Read()`. That means this PR goes through all callers (in some cases multiple layers up the call stack) to find a `ReadOptions` to provide the priority. There are TODOs for cases I believe it would be good to let user control the priority some day (e.g., file footer reads), and no TODO in cases I believe it doesn't matter (e.g., trace file reads). The API doc only lists the missing cases where a file read associated with a provided `ReadOptions` cannot be rate limited. For cases like file ingestion checksum calculation, there is no API to provide `ReadOptions` or `Env::IOPriority`, so I didn't count that as missing. Pull Request resolved: #9424 Test Plan: - new unit tests - new benchmarks on ~50MB database with 1MB/s read rate limit and 100ms refill interval; verified with strace reads are chunked (at 0.1MB per chunk) and spaced roughly 100ms apart. - setup command: `./db_bench -benchmarks=fillrandom,compact -db=/tmp/testdb -target_file_size_base=1048576 -disable_auto_compactions=true -file_checksum=true` - benchmarks command: `strace -ttfe pread64 ./db_bench -benchmarks=verifychecksum,verifyfilechecksums -use_existing_db=true -db=/tmp/testdb -rate_limiter_bytes_per_sec=1048576 -rate_limit_bg_reads=1 -rate_limit_user_ops=true -file_checksum=true` - crash test using IO_USER priority on non-validation reads with #9567 reverted: `python3 tools/db_crashtest.py blackbox --max_key=1000000 --write_buffer_size=524288 --target_file_size_base=524288 --level_compaction_dynamic_level_bytes=true --duration=3600 --rate_limit_bg_reads=true --rate_limit_user_ops=true --rate_limiter_bytes_per_sec=10485760 --interval=10` Reviewed By: hx235 Differential Revision: D33747386 Pulled By: ajkr fbshipit-source-id: a2d985e97912fba8c54763798e04f006ccc56e0c

pdillinger requested a review from mrambacher February 14, 2022 23:16

facebook-github-bot added the CLA Signed label Feb 14, 2022

pdillinger added 2 commits February 14, 2022 16:17

Fix static init order issue

415f1c7

Fix LITE and microbench

f382c52

mrambacher reviewed Feb 15, 2022

View reviewed changes

pdillinger added 2 commits February 15, 2022 08:53

Address feedback

d856cf8

Merge remote-tracking branch 'origin/main' into filter_policy_customi…

1337b32

…zable

Another make format ¯\_(ツ)_/¯

2938c7e

mrambacher reviewed Feb 15, 2022

View reviewed changes

Respond to feedback + lints

9ba719f

mrambacher approved these changes Feb 16, 2022

View reviewed changes

facebook-github-bot closed this in 8c68108 Feb 16, 2022

ajkr mentioned this pull request Feb 17, 2022

Add rate limiter priority to ReadOptions #9424

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor FilterPolicies toward Customizable #9567

Refactor FilterPolicies toward Customizable #9567

pdillinger commented Feb 14, 2022

facebook-github-bot commented Feb 15, 2022

mrambacher Feb 15, 2022

mrambacher Feb 15, 2022

pdillinger Feb 15, 2022

mrambacher Feb 15, 2022

pdillinger Feb 15, 2022

mrambacher Feb 16, 2022

pdillinger Feb 16, 2022

facebook-github-bot commented Feb 15, 2022

facebook-github-bot commented Feb 15, 2022

facebook-github-bot commented Feb 15, 2022

facebook-github-bot commented Feb 15, 2022

facebook-github-bot commented Feb 15, 2022

mrambacher left a comment

mrambacher Feb 16, 2022

pdillinger Feb 16, 2022


		inline int TEST_GetStartingLevelForB() const { return starting_level_for_b_; }
		static constexpr const char* kName = "bloomfilter";

Refactor FilterPolicies toward Customizable #9567

Refactor FilterPolicies toward Customizable #9567

Conversation

pdillinger commented Feb 14, 2022

facebook-github-bot commented Feb 15, 2022

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

facebook-github-bot commented Feb 15, 2022

facebook-github-bot commented Feb 15, 2022

facebook-github-bot commented Feb 15, 2022

facebook-github-bot commented Feb 15, 2022

facebook-github-bot commented Feb 15, 2022

mrambacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment