From 6a51af16b318fcfce45c2f2699a7b015280c7cf4 Mon Sep 17 00:00:00 2001 From: Hui Xiao Date: Mon, 16 May 2022 16:12:02 -0700 Subject: [PATCH] Feedback: fix API comment && remove unnecessary check --- include/rocksdb/table.h | 53 ++++++++++++------- .../block_based/block_based_table_builder.cc | 10 ++-- .../block_based/block_based_table_factory.cc | 10 ++-- 3 files changed, 39 insertions(+), 34 deletions(-) diff --git a/include/rocksdb/table.h b/include/rocksdb/table.h index 2b050e742..ec6c3cfd4 100644 --- a/include/rocksdb/table.h +++ b/include/rocksdb/table.h @@ -317,8 +317,8 @@ struct BlockBasedTableOptions { // `options_overrides[role].f != kFallback`, we use // `options_overrides[role].f` // 2. Otherwise, if `options[role].f != kFallback`, we use `options[role].f` - // 3. Otherwise, we follow the compatible existing behavior for `f` (see below - // for more) + // 3. Otherwise, we follow the compatible existing behavior for `f` (see + // each feature's comment for more) // // `cache_usage_options` currently supports specifying options for the // following features: @@ -329,42 +329,55 @@ struct BlockBasedTableOptions { // available), by updating a dynamical charge to the block cache loosely based // on the actual memory usage of that area. // - // Existing behavior: - // (1) CacheEntryRole::kCompressionDictionaryBuildingBuffer : kEnabled + // (a) CacheEntryRole::kCompressionDictionaryBuildingBuffer + // (i) If 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), the data will then be // unbuffered. + // (ii) If kDisabled: + // Does not charge the memory usage mentioned above. + // (iii) Compatible existing behavior: + // Same as kEnabled. // - // (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. + // (b) CacheEntryRole::kFilterConstruction + // (i) If kEnabled: + // Charge memory usage of Bloom Filter + // (format_version >= 5) and Ribbon Filter construction. + // 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. + // (ii) If kDisabled: + // Does not charge the memory usage mentioned above. + // (iii) Compatible existing behavior: + // Same as kDisabled. // - // (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) - // + some internal data structures during table reader creation. - // If enabled and if such a table reader exceeds + // (c) CacheEntryRole::kBlockBasedTableReader + // (i) If kEnabled: + // 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) + some internal data structures during table reader creation. + // 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(). + // (ii) If kDisabled: + // Does not charge the memory usage mentioned above. + // (iii) Compatible existing behavior: + // Same as kDisabled. // - // (4) Other CacheEntryRole : Not supported + // (d) Other CacheEntryRole + // Not supported. // `Status::kNotSupported` will be returned if // `CacheEntryRoleOptions::charged` is set to {`kEnabled`, `kDisabled`}. // // // 2. More to come ... // - // CacheUsageOptions cache_usage_options; // Note: currently this option requires kTwoLevelIndexSearch to be set as diff --git a/table/block_based/block_based_table_builder.cc b/table/block_based/block_based_table_builder.cc index f95d9f72c..32144e417 100644 --- a/table/block_based/block_based_table_builder.cc +++ b/table/block_based/block_based_table_builder.cc @@ -462,14 +462,10 @@ struct BlockBasedTableBuilder::Rep { compression_opts.max_dict_buffer_bytes); } - const auto options_overrides_iter = - table_options.cache_usage_options.options_overrides.find( - CacheEntryRole::kCompressionDictionaryBuildingBuffer); const auto compress_dict_build_buffer_charged = - options_overrides_iter != - table_options.cache_usage_options.options_overrides.end() - ? options_overrides_iter->second.charged - : table_options.cache_usage_options.options.charged; + table_options.cache_usage_options.options_overrides + .at(CacheEntryRole::kCompressionDictionaryBuildingBuffer) + .charged; if (table_options.block_cache && (compress_dict_build_buffer_charged == CacheEntryRoleOptions::Decision::kEnabled || diff --git a/table/block_based/block_based_table_factory.cc b/table/block_based/block_based_table_factory.cc index b959fe8c1..588c82236 100644 --- a/table/block_based/block_based_table_factory.cc +++ b/table/block_based/block_based_table_factory.cc @@ -422,14 +422,10 @@ BlockBasedTableFactory::BlockBasedTableFactory( InitializeOptions(); RegisterOptions(&table_options_, &block_based_table_type_info); - const auto options_overrides_iter = - table_options_.cache_usage_options.options_overrides.find( - CacheEntryRole::kBlockBasedTableReader); const auto table_reader_charged = - options_overrides_iter != - table_options_.cache_usage_options.options_overrides.end() - ? options_overrides_iter->second.charged - : table_options_.cache_usage_options.options.charged; + table_options_.cache_usage_options.options_overrides + .at(CacheEntryRole::kBlockBasedTableReader) + .charged; if (table_options_.block_cache && table_reader_charged == CacheEntryRoleOptions::Decision::kEnabled) { table_reader_cache_res_mgr_.reset(new ConcurrentCacheReservationManager(