Rewrite memory-charging feature's option API #9926

hx235 · 2022-05-01T05:23:25Z

Context:
Previous PR #9748, #9073, #8428 added separate flag for each charged memory area. Such API design is not scalable as we charge more and more memory areas. Also, we foresee an opportunity to consolidate this feature with other cache usage related features such as cache_index_and_filter_blocks using CacheEntryRole.

Therefore we decided to consolidate all these flags with CacheUsageOptions cache_usage_options and this PR serves as the first step by consolidating memory-charging related flags.

Summary:

Replaced old API reference with new ones, including making kCompressionDictionaryBuildingBuffer opt-out and added a unit test for that
Added missing db bench/stress test for some memory charging features
Renamed related test suite to indicate they are under the same theme of memory charging
Refactored a commonly used mocked cache component in memory charging related tests to reduce code duplication
Replaced the phrases "memory tracking" / "cache reservation" (other than CacheReservationManager-related ones) with "memory charging" for standard description of this feature.

Test plan:

New unit test for opt-out kCompressionDictionaryBuildingBuffer TEST_F(ChargeCompressionDictionaryBuildingBufferTest, Basic)
New unit test for option validation/sanitization TEST_F(CacheUsageOptionsOverridesTest, SanitizeAndValidateOptions)
CI
db bench (in case querying new options introduces regression) +0.5% micros/op: TEST_TMPDIR=/dev/shm/testdb ./db_bench -benchmarks=fillseq -db=$TEST_TMPDIR -charge_compression_dictionary_building_buffer=1(remove this for comparison) -compression_max_dict_bytes=10000 -disable_auto_compactions=1 -write_buffer_size=100000 -num=4000000 | egrep 'fillseq'

#-run	(pre-PR) avg micros/op	std micros/op	(post-PR) micros/op	std micros/op	change (%)
10	3.9711	0.264408	3.9914	0.254563	0.5111933721
20	3.83905	0.0664488	3.8251	0.0695456	-0.3633711465
40	3.86625	0.136669	3.8867	0.143765	0.5289363078

db_stress: python3 tools/db_crashtest.py blackbox -charge_compression_dictionary_building_buffer=1 -charge_filter_construction=1 -charge_table_reader=1 -cache_size=1 killed as normal

table/block_based/block_based_table_factory.cc

facebook-github-bot · 2022-05-01T18:44:01Z

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

ajkr · 2022-05-03T00:29:18Z

std::map<CacheEntryRole /* memory area */, bool /* is_tracked */> memory_tracking_override = {};

There's a fairly high chance the future memory tracking configuration is more complex than a bool. For that reason I'd wrap the bool in a struct and put the struct in the map's value. Assuming we do that, the bool becomes inflexible because users must decide tracking even if they really only want to override some other setting. For that reason I'd make an enum class TrackingState with a kFallback as one of the enum values.

hx235 · 2022-05-03T01:28:05Z

std::map<CacheEntryRole /* memory area */, bool /* is_tracked */> memory_tracking_override = {};

There's a fairly high chance the future memory tracking configuration is more complex than a bool. For that reason I'd wrap the bool in a struct and put the struct in the map's value. Assuming we do that, the bool becomes inflexible because users must decide tracking even if they really only want to override some other setting. For that reason I'd make an enum class TrackingState with a kFallback as one of the enum values.

Got you - let me look into that!

pdillinger · 2022-05-03T18:53:41Z

I think a lot of the changes here are based on an implementation distinction that I don't think the user should be exposed to or thinking about: pinned vs. tracked. For the user, things that use memory in RocksDB have one of four treatments:

The memory usage is charged to block cache but NOT ejectable from block cache. (Internally, this could mean pinned, likely immutable, or tracked, likely mutable.)
The memory usage is charged to block cache and IS ejectable from block cache though given HIGH priority treatment.
The memory usage is charged to block cache and IS ejectable from block cache and given LOW priority treatment.
The memory usage is NOT charged to block cache.

For example, if we decide to make table readers ejectable under block cache, the old state of affairs should not be "baked in" to the API.

facebook-github-bot · 2022-05-05T02:56:32Z

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

hx235 · 2022-05-05T02:56:46Z

Just CI - not ready for review.

facebook-github-bot · 2022-05-05T03:15:16Z

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

facebook-github-bot · 2022-05-05T05:24:37Z

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

facebook-github-bot · 2022-05-05T06:33:33Z

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

ajkr · 2022-05-10T23:25:06Z

db/db_sst_test.cc

+      const auto iter_1 =
+          table_options.cache_usage_options.options_overrides.find(
+              CacheEntryRole::kBlockBasedTableReader);


options_overrides_iter may be a more conventional name.

Fixed (it was chosen to quickly hack a PR to benchmark)

ajkr · 2022-05-10T23:27:40Z

db/db_sst_test.cc

+      // if (table_options.cache_usage_options
+      //         .options_overrides[static_cast<uint32_t>(
+      //             CacheEntryRole::kBlockBasedTableReader)]
+      //         .charged == CacheEntryRoleOptions::Decision::kEnabled) {


There's a lot of commented code that needs to be deleted (and some still needs to be migrated like in db_stress).

Fixed by removing the previous hacking code

ajkr · 2022-05-10T23:40:07Z

include/rocksdb/table.h

+  // `cache_usage_options` allows users to specify the default
+  // options (`cache_usage_options.default_options`) and the overrides
+  // options (`cache_usage_options.options_overrides`)


default_options and options_overrides doesn't feel like a perfect match to me since options_overrides sounds like things that'd override options. Some pairs that I like:

options and options_overrides

base_options and (role_to_options or options_by_role)

I'll go with the first suggestion - thanks!

ajkr · 2022-05-10T23:54:48Z

include/rocksdb/table.h

+  // For a certain `CacheEntryRole r` and a certain feature `f` of
+  // `CacheEntryRoleOptions`:
+  // (a) If `options_overrides[static_cast<uint32_t>(role)].f !=
+  // Decision::kFallback`, then it will be used to override the `Decision` value
+  // of `cache_usage_options.default_options.f`.
+  //
+  // Example:
+  // (a.1)
+  // `options_overrides[static_cast<uint32_t>(CacheEntryRole::kBlockBasedTableReader)]
+  // = {.charged = Decision::kEnabled}`
+  // (a.2) `cache_usage_options.default_options.charged = Decision::kFallback`
+  // (a.1) + (a.2) => the `CacheEntryRoleOptions::charged`
+  // feature will be enabled for `CacheEntryRole::kBlockBasedTableReader`
+  //
+  // (b) If `options_overrides[static_cast<uint32_t>(role)].f ==
+  // Decision::kFallback`, then the `Decision` value of
+  // `cache_usage_options.default_options.f` will be used. If this default value
+  // is set to `Decision::kFallback`, then the existing behavior will be in
+  // effect. See "Features" below for the existing behaviors.
+  //
+  // Example:
+  // (b.1)
+  // `options_overrides[static_cast<uint32_t>(CacheEntryRole::kBlockBasedTableReader)]
+  // = {.charged = Decision::kFallback}`
+  // (b.2) `cache_usage_options.default_options.charged = Decision::kFallback`
+  // (b.1) + (b.2) => the `CacheEntryRoleOptions::charged` feature will fallback
+  // to the existing behavior for `CacheEntryRole::kBlockBasedTableReader`


Would it be simpler to list the items in order of preference?

If options_overrides has an entry for role and options_overrides[role].f != kFallback, we use options_overrides[role].f

Otherwise, if options[role].f != kFallback, we use options[role].f

Otherwise, we follow the compatible existing behavior.

The comment is basically a version of this for std::array (where we don't have "If options_overrides has an entry for role"). But my example and description might be too wordy so let me fix that with your style of listing.

(And I realized your suggestion has the merit in " if options[role].f != kFallback, we use options[role].f" which was what I didn't have in my doc)

ajkr · 2022-05-11T00:09:42Z

include/rocksdb/table.h

+  // (2) CacheEntryRole::kFilterConstruction : kDisabled
+  // If enabled, charge memory usage of Bloom Filter (format_version >= 5) and


Since this is describing existing behavior I'd say something like "Same as kDisabled - does not charge memory usage of ...".

ajkr · 2022-05-11T00:37:11Z

tools/db_bench_tool.cc

+      block_based_options.cache_usage_options.options_overrides.insert(
+          {CacheEntryRole::kFilterConstruction,
+           {.charged = FLAGS_charge_filter_construction
+                           ? CacheEntryRoleOptions::Decision::kEnabled
+                           : CacheEntryRoleOptions::Decision::kDisabled}});
+      block_based_options.cache_usage_options.options_overrides.insert(
+          {CacheEntryRole::kFilterConstruction,
+           {.charged = FLAGS_charge_filter_construction
+                           ? CacheEntryRoleOptions::Decision::kEnabled
+                           : CacheEntryRoleOptions::Decision::kDisabled}});


These two are the same.

ajkr · 2022-05-11T00:42:59Z

table/table_test.cc

+  table_options.cache_usage_options.options_overrides.insert(
+      {CacheEntryRole::kCompressionDictionaryBuildingBuffer,
+       {.charged = CacheEntryRoleOptions::Decision::kEnabled}});


Is it needed? It's the default behavior to charge for it, right?

It is not needed - it was there instead of a comment like "CacheEntryRoleOptions::charged is enabled by default for
CacheEntryRole::kCompressionDictionaryBuildingBuffer".

But I am removing it here as suggested to for more testing coverage.

ajkr · 2022-05-11T00:48:52Z

table/block_based/filter_policy.cc

+      iter != context.table_options.cache_usage_options.options_overrides.end()
+          ? iter->second.charged == CacheEntryRoleOptions::Decision::kEnabled
+          : context.table_options.cache_usage_options.default_options.charged ==
+                CacheEntryRoleOptions::Decision::kEnabled;


This expression scares me, originally because of its length/density and later because it appears to be assigning a temporary boolean to a reference. I guess the const bool&'s lifetime will be extended and it's indeed correct but it is too much extra difficulty to understand. Can we add an if-else and remove the reference?

Fixed by comparing .charged with CacheEntryRoleOptions::Decision instead of bool.

facebook-github-bot · 2022-05-11T21:13:22Z

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

hx235 · 2022-05-11T21:13:32Z

Not ready for review - just going through CI

facebook-github-bot · 2022-05-11T21:21:15Z

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

facebook-github-bot · 2022-05-11T22:08:04Z

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

facebook-github-bot · 2022-05-11T22:10:45Z

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

facebook-github-bot · 2022-05-11T22:55:03Z

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

hx235 · 2022-05-11T23:55:10Z

@ajkr ready for review again!

facebook-github-bot · 2022-05-12T00:14:24Z

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

facebook-github-bot · 2022-05-12T00:14:46Z

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

ajkr

Looks great, thanks for introducing this block cache configuration API!

ajkr · 2022-05-14T02:00:34Z

include/rocksdb/table.h

  // reader (i.e, BlockBasedTableOptions::cache_index_and_filter_blocks ==
  // false)
-  // 3. Some internal data structures
-  // 4. More to come...
+  // + some internal data structures during table reader creation.


some internal data structures during table reader creation.

If we want to mention "some internal data structures" can it be moved together with "table properties + index block/filter block/uncompression dictionary"?

ajkr · 2022-05-14T02:08:46Z

include/rocksdb/table.h

+  // (1) CacheEntryRole::kCompressionDictionaryBuildingBuffer : kEnabled
+  // Charge memory usage of the buffered data used as training samples for
+  // dictionary compression.
+  // If such memory usage exceeds the avaible space left in the block cache
  // at some point (i.e, causing a cache full under
-  // LRUCacheOptions::strict_capacity_limit = true), construction will fall back
-  // to Bloom Filter.
-  //
-  // Default: false
-  bool reserve_table_builder_memory = false;
-
-  // If true, a dynamically updating charge to block cache, loosely based
-  // on the actual memory usage of table reader, will occur to account
-  // the memory, if block cache available.
-  //
-  // Charged memory usage includes:
-  // 1. Table properties
-  // 2. Index block/Filter block/Uncompression dictionary if stored in table
+  // `LRUCacheOptions::strict_capacity_limit` = true), the data will then be
+  // unbuffered.
+  //
+  // (2) CacheEntryRole::kFilterConstruction : kDisabled
+  // Same as kDisabled - does not charge memory usage of Bloom Filter
+  // (format_version >= 5) and Ribbon Filter construction. If enabled and if
+  // additional temporary memory of Ribbon Filter exceeds the avaible space left
+  // in the block cache at some point (i.e, causing a cache full under
+  // `LRUCacheOptions::strict_capacity_limit` = true), construction will fall
+  // back to Bloom Filter.
+  //
+  // (3) CacheEntryRole::kBlockBasedTableReader : kDisabled
+  // Same as kDisabled -  does not charge memory usage of table properties +
+  // index block/filter block/uncompression dictionary when stored in table
  // reader (i.e, BlockBasedTableOptions::cache_index_and_filter_blocks ==
  // false)
-  // 3. Some internal data structures
-  // 4. More to come...
+  // + some internal data structures during table reader creation.
+  // If enabled and if such a table reader exceeds
+  // the avaible space left in the block cache at some point (i.e, causing
+  // a cache full under `LRUCacheOptions::strict_capacity_limit` = true),
+  // creation will fail with Status::MemoryLimit().
+  //
+  // (4) Other CacheEntryRole : Not supported
+  // `Status::kNotSupported` will be returned if
+  // `CacheEntryRoleOptions::charged` is set to {`kEnabled`, `kDisabled`}.


Hm this feels like it contains instructions on how to use it that don't really belong in an "Existing behavior:" subsection. Perhaps we can make these subitems directly under "Memory charging to block cache". Each subitem can have three bullet points - what does kDisabled mean, what does kEnabled mean, and what is the compatible existing behavior (e.g., "Same as kDisabled"). Also can we label the subitems like (a), (b), (c), as I initially thought this is a totally separate list from the one starting with 1.?

ajkr · 2022-05-14T05:20:56Z

table/block_based/block_based_table_factory.cc

+          CacheEntryRole::kBlockBasedTableReader);
+  const auto table_reader_charged =
+      options_overrides_iter !=
+              table_options_.cache_usage_options.options_overrides.end()


The InitializeOptions() logic looks like it populates entries for every CacheEntryRole including filling in fallbacks to default options. In that case can we use table_options_.cache_usage_options.options_overrides.at(CacheEntryRole::kBlockBasedTableReader).charged to get the final result directly? (Same question applies to similar code like for calculating filter_construction_charged.)

For CacheEntryRole::kBlockBasedTableReader, yeah we should remove (and already fixed it)
For filter_construction_charged, some tests directly initialize with FilterBitsBuilder without going through InitializeOptions like https://github.com/facebook/rocksdb/blob/7.2.fb/util/bloom_test.cc#L301 so we can't do so without fixing a lot of the tests (so I will leave it as it is)
For dictionary compression buffer, still going through CI check and see if there is any test we need to fix for that.

ok with me. Sounds like an issue with the tests if they are using internal APIs in a way that could not happen in non-test code.

facebook-github-bot · 2022-05-16T23:13:52Z

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

facebook-github-bot · 2022-05-16T23:33:09Z

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

facebook-github-bot · 2022-05-17T17:21:29Z

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

Summary: **Context/Summary:** #9926 removed inefficient `reserve*` option API but forgot to mark them deprecated in `block_based_table_type_info` for compatible table format. Pull Request resolved: #10016 Test Plan: build-format-compatible Reviewed By: pdillinger Differential Revision: D36484247 Pulled By: hx235 fbshipit-source-id: c41b90cc99fb7ab7098934052f0af7290b221f98

hx235 added the WIP Work in progress label May 1, 2022

facebook-github-bot added the CLA Signed label May 1, 2022

hx235 changed the title ~~Rewrite memory tracking API~~ Rewrite memory tracking feature's API May 1, 2022

hx235 force-pushed the memory_tracking_api branch 2 times, most recently from ae0463f to 5c9d23d Compare May 1, 2022 05:36

hx235 commented May 1, 2022

View reviewed changes

table/block_based/block_based_table_factory.cc Outdated Show resolved Hide resolved

hx235 force-pushed the memory_tracking_api branch 3 times, most recently from e366b28 to f01eb04 Compare May 1, 2022 07:15

hx235 mentioned this pull request May 1, 2022

Account memory of FileMetaData in global memory limit #9924

Closed

hx235 force-pushed the memory_tracking_api branch 2 times, most recently from 1bfca26 to 750f0f2 Compare May 1, 2022 18:16

hx235 changed the title ~~Rewrite memory tracking feature's API~~ Rewrite memory-tracking feature's option API May 2, 2022

hx235 requested a review from ajkr May 2, 2022 16:35

hx235 removed the WIP Work in progress label May 2, 2022

hx235 force-pushed the memory_tracking_api branch from 750f0f2 to ca2b767 Compare May 5, 2022 02:56

hx235 added the WIP Work in progress label May 5, 2022

hx235 force-pushed the memory_tracking_api branch from ca2b767 to c564883 Compare May 5, 2022 03:15

hx235 force-pushed the memory_tracking_api branch from c564883 to 41ec5d7 Compare May 5, 2022 05:24

hx235 force-pushed the memory_tracking_api branch from 41ec5d7 to ccbb517 Compare May 5, 2022 06:33

hx235 changed the title ~~Rewrite memory-tracking feature's option API~~ Rewrite memory-charging feature's option API May 5, 2022

ajkr reviewed May 11, 2022

View reviewed changes

hx235 force-pushed the memory_tracking_api branch from 3199ea9 to c2b2610 Compare May 11, 2022 21:13

hx235 force-pushed the memory_tracking_api branch from c2b2610 to ac0e607 Compare May 11, 2022 21:21

hx235 force-pushed the memory_tracking_api branch from ac0e607 to 64d3b21 Compare May 11, 2022 22:08

hx235 force-pushed the memory_tracking_api branch from 64d3b21 to f13d55d Compare May 11, 2022 22:10

hx235 removed the WIP Work in progress label May 11, 2022

hx235 force-pushed the memory_tracking_api branch from f13d55d to 6c3baad Compare May 12, 2022 00:14

hx235 requested a review from ajkr May 12, 2022 00:14

ajkr approved these changes May 14, 2022

View reviewed changes

hx235 added 2 commits May 16, 2022 16:13

Rewrite memory charging option API

e214d4e

Feedback: use map for internal rep & miscs

eeff4a8

hx235 force-pushed the memory_tracking_api branch from 6c3baad to b7306e4 Compare May 16, 2022 23:13

Feedback: fix API comment && remove unnecessary check

6a51af1

hx235 force-pushed the memory_tracking_api branch from b7306e4 to 6a51af1 Compare May 16, 2022 23:33

facebook-github-bot closed this in 3573558 May 17, 2022

hx235 mentioned this pull request May 18, 2022

Mark old reserve* option deprecated #10016

Closed

		// (2) CacheEntryRole::kFilterConstruction : kDisabled
		// If enabled, charge memory usage of Bloom Filter (format_version >= 5) and

Rewrite memory-charging feature's option API #9926

Rewrite memory-charging feature's option API #9926

Conversation

hx235 commented May 1, 2022 • edited

facebook-github-bot commented May 1, 2022

ajkr commented May 3, 2022

hx235 commented May 3, 2022

pdillinger commented May 3, 2022

facebook-github-bot commented May 5, 2022

hx235 commented May 5, 2022

facebook-github-bot commented May 5, 2022

facebook-github-bot commented May 5, 2022

facebook-github-bot commented May 5, 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

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

hx235 May 11, 2022 • edited

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 May 11, 2022

hx235 commented May 11, 2022

facebook-github-bot commented May 11, 2022

facebook-github-bot commented May 11, 2022

facebook-github-bot commented May 11, 2022

facebook-github-bot commented May 11, 2022

hx235 commented May 11, 2022

facebook-github-bot commented May 12, 2022

facebook-github-bot commented May 12, 2022

ajkr left a comment • edited

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

facebook-github-bot commented May 16, 2022

facebook-github-bot commented May 16, 2022

facebook-github-bot commented May 17, 2022

hx235 commented May 1, 2022 •

edited

hx235 May 11, 2022 •

edited

ajkr left a comment •

edited