Use block cache deleters for type inspection and checking #8276

pdillinger · 2021-05-06T21:52:05Z

Summary: Along with #8225, this is working toward gathering statistics
about the kinds of items in block cache by iterating over the cache
during periodic stats dump. We need some way to flag what kind of
blocks each entry belongs to, preferably without changing the Cache API.
One of the complications is that Cache is a general interface that could
have other users that don't adhere to whichever convention we decide
on for keys and values. Or we would pay for an extra field in the Handle
that would only be used for this purpose.

This change uses a back-door approach, the deleter, to mark the kind
of Cache entries. This has the added benefit of ensuring that each entry
we check the kind of has the proper code origin; if the entry came from
some other part of the code, it will use an unrecognized deleter, which
we can simply attribute to an "unknown" bucket.

In detail, this change pulls the awkward BlocklikeTraits into
CachableEntry, along with an API for translating between a deleter and
<CacheEntryType, BlockType>. This should soon be used for collecting
stats.

A side benefit/feature in this change is that the information in the
deleter can be used for "type checking" block cache accesses, through
a new option extra_block_cache_checks. Any logical error that could lead
to a block cache key collision can break type safety, such as with a
bad cast from Block to BlockContents. This option detects and reports
such errors before the bad cast. This is now always enabled for
db_stress, and for one CircleCI unit test configuration, to ensure we
stay clean, but otherwise disabled by default because of modest
performance penalty.

Test Plan: TODO: small unit tests and performance tests

Summary: Along with facebook#8225, this is working toward gathering statistics about the kinds of items in block cache by iterating over the cache during periodic stats dump. We need some way to flag what kind of blocks each entry belongs to, preferably without changing the Cache API. One of the complications is that Cache is a general interface that could have other users that don't adhere to whichever convention we decide on for keys and values. Or we would pay for an extra field in the Handle that would only be used for this purpose. This change uses a back-door approach, the deleter, to mark the kind of Cache entries. This has the added benefit of ensuring that each entry we check the kind of has the proper code origin; if the entry came from some other part of the code, it will use an unrecognized deleter, which we can simply attribute to an "unknown" bucket. In detail, this change pulls the awkward BlocklikeTraits into CachableEntry, along with an API for translating between a deleter and `<CacheEntryType, BlockType>`. This should soon be used for collecting stats. A side benefit/feature in this change is that the information in the deleter can be used for "type checking" block cache accesses, through a new option extra_block_cache_checks. Any logical error that could lead to a block cache key collision can break type safety, such as with a bad cast from Block to BlockContents. This option detects and reports such errors before the bad cast. This is now always enabled for db_stress, and for one CircleCI unit test configuration, to ensure we stay clean, but otherwise disabled by default because of modest performance penalty. Test Plan: TODO: small unit tests and performance tests

ltamasi · 2021-05-12T01:33:10Z

table/block_based/block_based_table_reader.cc

-template <typename TBlocklike>
-class BlocklikeTraits;


I don't think this traits class/template is awkward at all; it's actually a standard generic programming technique.

CachableEntry and BlocklikeTraits have very little in common and have completely different roles and responsibilities: the former is a resource management object, while the latter is a textbook traits class/template. I don't think it makes sense to merge them.

I think I agree with your suggestion on noting that CachableEntry does not have any specific relationship to block-based table (despite being in table/block-based and only being instantiated for it) and noting that my extensions are essentially all static and specific to block-based table. But I still intend to reduce LoC in block_based_table_reader.cc ;)

Sure, block_based_table_reader.cc is still definitely too crowded. How about keeping the classes separate and just moving BlocklikeTraits to its own file(s)?

ltamasi · 2021-05-12T01:50:46Z

table/block_based/cachable_entry.cc

+template <class T>
+void PopulateDeleterMap(DeleterMap* map) {
+  const CacheEntryType entry_type = CachableEntry<T>::GetType();
+
+#define HANDLE_CASE(kBlockType)                                  \
+  assert(map->find(UniqueDeleter<T, kBlockType>) == map->end()); \
+  (*map)[UniqueDeleter<T, kBlockType>] = {entry_type, kBlockType}
+
+  HANDLE_CASE(BlockType::kData);
+  HANDLE_CASE(BlockType::kFilter);
+  HANDLE_CASE(BlockType::kProperties);
+  HANDLE_CASE(BlockType::kCompressionDictionary);
+  HANDLE_CASE(BlockType::kRangeDeletion);
+  HANDLE_CASE(BlockType::kHashIndexPrefixes);
+  HANDLE_CASE(BlockType::kHashIndexMetadata);
+  HANDLE_CASE(BlockType::kMetaIndex);
+  HANDLE_CASE(BlockType::kIndex);
+#undef HANDLE_CASE
+}
+
+DeleterMap ComputeDeleterMap() {
+  DeleterMap rv;
+  PopulateDeleterMap<BlockContents>(&rv);
+  PopulateDeleterMap<ParsedFullFilterBlock>(&rv);
+  PopulateDeleterMap<Block>(&rv);
+  PopulateDeleterMap<UncompressionDict>(&rv);
+  return rv;
+}


Many of the combinations generated here are "invalid"; e.g. the data, index, properties, range deletion, ... blocks are always of the "parsed KV block" variety. It might make sense to instantiate only the valid ones.

ltamasi · 2021-05-12T02:26:21Z

table/block_based/cachable_entry.cc

+template <class T, BlockType kBlockType>
+void UniqueDeleter(const Slice& /*key*/, void* value) {
+  auto entry = reinterpret_cast<T*>(value);
+  delete entry;
+}


For stats ticking purposes, I assume the plan is to take the Statistics pointer from T ?

That's not the plan, instead "gathering statistics about the kinds of items in block cache by iterating over the cache during periodic stats dump," referring to InternalStats

Got it, so you're just using the deleters to encode type information

Summary: This change gathers statistics about the kinds of items in block cache by iterating over the cache during periodic stats dump (InternalStats, stats_dump_period_sec). This is especially important for profiling relative usage of cache by index vs. filter vs. data blocks. This change only dumps the information to info log, for example: Block cache LRUCache@0x7f77f1029330 capacity: 95.37 MB collections: 8 last_copies: 0 last_secs: 0.001848 Block cache entry stats(count,size,portion): DataBlock(7092,28.24 MB,29.6136%) FilterBlock(215,867.90 KB,0.888728%) FilterMetaBlock(2,5.31 KB,0.00544%) IndexBlock(217,180.11 KB,0.184432%) WriteBuffer(1,256.00 KB,0.262144%) Misc(1,0.00 KB,0%) Follow-up work will make the data accessible through public APIs. Solution detail - We need some way to flag what kind of blocks each entry belongs to, preferably without changing the Cache API. One of the complications is that Cache is a general interface that could have other users that don't adhere to whichever convention we decide on for keys and values. Or we would pay for an extra field in the Handle that would only be used for this purpose. This change uses a back-door approach, the deleter, to indicate the "role" of a Cache entry (in addition to the value type, implicitly). This has the added benefit of ensuring proper code origin whenever we recognize a particular role for a cache entry; if the entry came from some other part of the code, it will use an unrecognized deleter, which we simply attribute to the "Misc" role. An internal API makes for simple instantiation and automatic registration of Cache deleters for a given value type and "role". Another internal API, CacheEntryStatsCollector, solves the problem of caching the results of a scan and sharing them, to ensure scans are neither excessive nor redundant so as not to harm Cache performance. Because code is added to BlocklikeTraits, it is pulled out of block_based_table_reader.cc into its own file. This is a reformulation of facebook#8276, without the type checking option (could still be added), and with actual stat gathering. Test Plan: manual testing with db_bench TODO: some unit tests

pdillinger · 2021-05-19T15:26:49Z

Replaced by #8297

Summary: This change gathers and publishes statistics about the kinds of items in block cache. This is especially important for profiling relative usage of cache by index vs. filter vs. data blocks. It works by iterating over the cache during periodic stats dump (InternalStats, stats_dump_period_sec) or on demand when DB::Get(Map)Property(kBlockCacheEntryStats), except that for efficiency and sharing among column families, saved data from the last scan is used when the data is not considered too old. The new information can be seen in info LOG, for example: Block cache LRUCache@0x7fca62229330 capacity: 95.37 MB collections: 8 last_copies: 0 last_secs: 0.00178 secs_since: 0 Block cache entry stats(count,size,portion): DataBlock(7092,28.24 MB,29.6136%) FilterBlock(215,867.90 KB,0.888728%) FilterMetaBlock(2,5.31 KB,0.00544%) IndexBlock(217,180.11 KB,0.184432%) WriteBuffer(1,256.00 KB,0.262144%) Misc(1,0.00 KB,0%) And also through DB::GetProperty and GetMapProperty (here using ldb just for demonstration): $ ./ldb --db=/dev/shm/dbbench/ get_property rocksdb.block-cache-entry-stats rocksdb.block-cache-entry-stats.bytes.data-block: 0 rocksdb.block-cache-entry-stats.bytes.deprecated-filter-block: 0 rocksdb.block-cache-entry-stats.bytes.filter-block: 0 rocksdb.block-cache-entry-stats.bytes.filter-meta-block: 0 rocksdb.block-cache-entry-stats.bytes.index-block: 178992 rocksdb.block-cache-entry-stats.bytes.misc: 0 rocksdb.block-cache-entry-stats.bytes.other-block: 0 rocksdb.block-cache-entry-stats.bytes.write-buffer: 0 rocksdb.block-cache-entry-stats.capacity: 8388608 rocksdb.block-cache-entry-stats.count.data-block: 0 rocksdb.block-cache-entry-stats.count.deprecated-filter-block: 0 rocksdb.block-cache-entry-stats.count.filter-block: 0 rocksdb.block-cache-entry-stats.count.filter-meta-block: 0 rocksdb.block-cache-entry-stats.count.index-block: 215 rocksdb.block-cache-entry-stats.count.misc: 1 rocksdb.block-cache-entry-stats.count.other-block: 0 rocksdb.block-cache-entry-stats.count.write-buffer: 0 rocksdb.block-cache-entry-stats.id: LRUCache@0x7f3636661290 rocksdb.block-cache-entry-stats.percent.data-block: 0.000000 rocksdb.block-cache-entry-stats.percent.deprecated-filter-block: 0.000000 rocksdb.block-cache-entry-stats.percent.filter-block: 0.000000 rocksdb.block-cache-entry-stats.percent.filter-meta-block: 0.000000 rocksdb.block-cache-entry-stats.percent.index-block: 2.133751 rocksdb.block-cache-entry-stats.percent.misc: 0.000000 rocksdb.block-cache-entry-stats.percent.other-block: 0.000000 rocksdb.block-cache-entry-stats.percent.write-buffer: 0.000000 rocksdb.block-cache-entry-stats.secs_for_last_collection: 0.000052 rocksdb.block-cache-entry-stats.secs_since_last_collection: 0 Solution detail - We need some way to flag what kind of blocks each entry belongs to, preferably without changing the Cache API. One of the complications is that Cache is a general interface that could have other users that don't adhere to whichever convention we decide on for keys and values. Or we would pay for an extra field in the Handle that would only be used for this purpose. This change uses a back-door approach, the deleter, to indicate the "role" of a Cache entry (in addition to the value type, implicitly). This has the added benefit of ensuring proper code origin whenever we recognize a particular role for a cache entry; if the entry came from some other part of the code, it will use an unrecognized deleter, which we simply attribute to the "Misc" role. An internal API makes for simple instantiation and automatic registration of Cache deleters for a given value type and "role". Another internal API, CacheEntryStatsCollector, solves the problem of caching the results of a scan and sharing them, to ensure scans are neither excessive nor redundant so as not to harm Cache performance. Because code is added to BlocklikeTraits, it is pulled out of block_based_table_reader.cc into its own file. This is a reformulation of #8276, without the type checking option (could still be added), and with actual stat gathering. Pull Request resolved: #8297 Test Plan: manual testing with db_bench, and a couple of basic unit tests Reviewed By: ltamasi Differential Revision: D28488721 Pulled By: pdillinger fbshipit-source-id: 472f524a9691b5afb107934be2d41d84f2b129fb

pdillinger requested a review from ltamasi May 6, 2021 21:52

facebook-github-bot added the CLA Signed label May 6, 2021

ltamasi reviewed May 12, 2021

View reviewed changes

pdillinger mentioned this pull request May 14, 2021

Use deleters to label cache entries and collect stats #8297

Closed

pdillinger closed this May 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use block cache deleters for type inspection and checking #8276

Use block cache deleters for type inspection and checking #8276

pdillinger commented May 6, 2021

ltamasi May 12, 2021

pdillinger May 12, 2021

ltamasi May 12, 2021

ltamasi May 12, 2021

ltamasi May 12, 2021

pdillinger May 12, 2021

ltamasi May 12, 2021

pdillinger commented May 19, 2021

Use block cache deleters for type inspection and checking #8276

Use block cache deleters for type inspection and checking #8276

Conversation

pdillinger commented May 6, 2021

ltamasi May 12, 2021

Choose a reason for hiding this comment

pdillinger May 12, 2021

Choose a reason for hiding this comment

ltamasi May 12, 2021

Choose a reason for hiding this comment

ltamasi May 12, 2021

Choose a reason for hiding this comment

ltamasi May 12, 2021

Choose a reason for hiding this comment

pdillinger May 12, 2021

Choose a reason for hiding this comment

ltamasi May 12, 2021

Choose a reason for hiding this comment

pdillinger commented May 19, 2021