Remove compressed block cache #11117

siying · 2023-01-24T04:09:10Z

Summary:
Compressed block cache is replaced by compressed secondary cache. Remove the feature.

Test Plan: See CI passes

facebook-github-bot · 2023-01-24T04:09:27Z

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

facebook-github-bot · 2023-01-24T20:04:09Z

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

pdillinger · 2023-01-24T20:16:56Z

include/rocksdb/statistics.h

@@ -188,12 +188,6 @@ enum Tickers : uint32_t {
  // Record the number of calls to GetUpdatesSince. Useful to keep track of
  // transaction log iterator refreshes
  GET_UPDATES_SINCE_CALLS,
-  BLOCK_CACHE_COMPRESSED_MISS,  // miss in the compressed block cache


I'm not sure of the consequences of re-numbering existing statistics. @ltamasi

Right, enum values on the C++ and Java sides have to be kept in sync.

Hm, actually on closer inspection, the ids are already out of sync in various ways. Seems like we first diverge at BLOOM_FILTER_FULL_POSITIVE, which would be 21 == 0x15 in the C++ code but missing from the JNI (where 0x15 is actually PERSISTENT_CACHE_HIT). Probably stuff got added in the middle at some point.

EDIT: BLOOM_FILTER_FULL_POSITIVE is actually there further down, with a different id.

Too bad I just spent half an hour changing the numbers and filled the gap...

It definitely would have been nice if the values had been kept in sync but that apparently that ship has sailed; even if we fill this gap, there are pre-existing tickers which have different ids between C++ and Java (e.g. BLOOM_FILTER_FULL_POSITIVE is 0x63 on the Java side).

There's also this comment in portal.h; I'm not sure what the original rationale behind exposing TICKER_ENUM_MAX was (especially since its value is guaranteed to change in the core library whenever a ticker is added) but seems like the convention in the Java bindings is to keep the ids the same across versions:

case ROCKSDB_NAMESPACE::Tickers::TICKER_ENUM_MAX: // 0x5F was the max value in the initial copy of tickers to Java. // Since these values are exposed directly to Java clients, we keep // the value the same forever. // // TODO: This particular case seems confusing and unnecessary to pin the // value since it's meant to be the number of tickers, not an actual // ticker value. But we aren't yet in a position to fix it since the // number of tickers doesn't fit in the Java representation (jbyte). return 0x5F;

I'm more concerned about if dependent internal code is tied to the ordinal numbers. Can take discussion to https://www.internalfb.com/diff/D42700164

Just did some digging in this area while working on removing some (other) stats - I'm fairly confident renumbering on the C++ side is safe because it has (organically) happened many times over the years as people added new tickers near related existing ones (as opposed to the end of the list). The Java bindings zag where the core library zigs though.

pdillinger · 2023-01-24T20:18:13Z

java/src/main/java/org/rocksdb/TickerType.java

-    /**
-     * Miss in the compressed block cache.
-     */
-    BLOCK_CACHE_COMPRESSED_MISS((byte) 0x42),


Probably wrong for new numbers

I don't think it would be wrong, as in portal.h they are just hard coded in the same way. I updated the PR and removed the gap anyway to be safer.

pdillinger · 2023-01-24T20:21:13Z

table/block_based/block_based_table_factory.cc

@@ -539,12 +524,6 @@ Status CheckCacheOptionCompatibility(const BlockBasedTableOptions& bbto) {
        ->Insert(sentinel_key.AsSlice(), &kRegularBlockCacheMarker, &kHelper, 1)
        .PermitUncheckedError();
  }
-  if (bbto.block_cache_compressed) {
-    bbto.block_cache_compressed
-        ->Insert(sentinel_key.AsSlice(), &kCompressedBlockCacheMarker, &kHelper,


A bit more to remove here, kCompressedBlockCacheMarker and related code below

facebook-github-bot · 2023-01-24T20:26:29Z

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

facebook-github-bot · 2023-01-24T20:54:35Z

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

pdillinger · 2023-01-24T21:49:36Z

Can you re-import? GitHub won't show me what has changed since my review, presumably because of the force-push. Thanks

facebook-github-bot · 2023-01-24T21:50:24Z

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

facebook-github-bot · 2023-01-24T21:51:11Z

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

siying · 2023-01-24T21:51:29Z

Can you re-import? GitHub won't show me what has changed since my review, presumably because of the force-push. Thanks

I just reimported.

pdillinger

I'm not sure about downstream breakage if we re-number stats, but otherwise LGTM

pdillinger · 2023-01-24T22:58:22Z

table/block_based/block_based_table_factory.cc

@@ -232,8 +232,6 @@ static std::unordered_map<std::string, OptionTypeInfo>
    block_based_table_type_info = {
 #ifndef ROCKSDB_LITE
        /* currently not supported
-          std::shared_ptr<Cache> block_cache = nullptr;


Please keep this line, to remind us that block_cache is not dynamically configurable

pdillinger · 2023-01-24T23:03:08Z

One thing you might consider is separating out the stat removals & Java numbering updates into another PR, in case we need an easy revert path.

Summary: Compressed block cache is replaced by compressed secondary cache. Remove the feature. Test Plan: See CI passes

facebook-github-bot · 2023-01-25T00:03:07Z

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

facebook-github-bot · 2023-01-25T00:03:27Z

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

siying · 2023-01-25T00:55:35Z

Add back to statistics and the comment line deleted by mistake.

facebook-github-bot · 2023-01-25T02:03:08Z

This pull request has been merged in 2800aa0.

This reverts commit 2800aa0.

facebook-github-bot added the CLA Signed label Jan 24, 2023

pdillinger self-requested a review January 24, 2023 20:12

pdillinger requested changes Jan 24, 2023

View reviewed changes

siying force-pushed the remove_compressed_c branch from 858d36b to bc7f338 Compare January 24, 2023 20:26

pdillinger approved these changes Jan 24, 2023

View reviewed changes

siying added 8 commits January 24, 2023 16:02

Remove compressed block cache

ca1d9fe

Summary: Compressed block cache is replaced by compressed secondary cache. Remove the feature. Test Plan: See CI passes

Fix

d7d67cc

Again fix Java

82ce6b8

Again fix Java

6b159c9

Fix

320208e

Change stats numbers.

237fec5

Revert stats

dbe24ff

Recover mistakely removed comment

58f532e

siying force-pushed the remove_compressed_c branch from 8d82fd2 to 58f532e Compare January 25, 2023 00:03

facebook-github-bot closed this in 2800aa0 Jan 25, 2023

facebook-github-bot added the Merged label Jan 25, 2023

ajkr added a commit that referenced this pull request Feb 14, 2023

Revert "Remove compressed block cache (#11117)"

14fef92

This reverts commit 2800aa0.

ajkr added a commit that referenced this pull request Feb 18, 2023

Revert "Remove compressed block cache (#11117)"

0771260

This reverts commit 2800aa0.

jamesbibby mentioned this pull request Jun 22, 2023

Rocksdb 8 Support DataDog/gorocksdb#28

Open

jamesbibby mentioned this pull request Oct 27, 2023

Add support for Rocksdb 8 DataDog/gorocksdb#33

Closed

igorcanadi mentioned this pull request Jan 17, 2024

[SYS-6913] Upgrade RocksDB-Cloud to 8.9.1 rockset/rocksdb-cloud#315

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove compressed block cache #11117

Remove compressed block cache #11117

siying commented Jan 24, 2023

facebook-github-bot commented Jan 24, 2023

facebook-github-bot commented Jan 24, 2023

pdillinger Jan 24, 2023

ltamasi Jan 24, 2023

ltamasi Jan 24, 2023 •

edited

Loading

siying Jan 24, 2023

ltamasi Jan 24, 2023 •

edited

Loading

pdillinger Jan 24, 2023

ltamasi Jan 24, 2023 •

edited

Loading

pdillinger Jan 24, 2023

siying Jan 24, 2023

pdillinger Jan 24, 2023

facebook-github-bot commented Jan 24, 2023

facebook-github-bot commented Jan 24, 2023

pdillinger commented Jan 24, 2023

facebook-github-bot commented Jan 24, 2023

facebook-github-bot commented Jan 24, 2023

siying commented Jan 24, 2023

pdillinger left a comment

pdillinger Jan 24, 2023

pdillinger commented Jan 24, 2023

facebook-github-bot commented Jan 25, 2023

facebook-github-bot commented Jan 25, 2023

siying commented Jan 25, 2023

facebook-github-bot commented Jan 25, 2023

Remove compressed block cache #11117

Remove compressed block cache #11117

Conversation

siying commented Jan 24, 2023

facebook-github-bot commented Jan 24, 2023

facebook-github-bot commented Jan 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltamasi Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltamasi Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltamasi Jan 24, 2023 • edited Loading

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 Jan 24, 2023

facebook-github-bot commented Jan 24, 2023

pdillinger commented Jan 24, 2023

facebook-github-bot commented Jan 24, 2023

facebook-github-bot commented Jan 24, 2023

siying commented Jan 24, 2023

pdillinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdillinger commented Jan 24, 2023

facebook-github-bot commented Jan 25, 2023

facebook-github-bot commented Jan 25, 2023

siying commented Jan 25, 2023

facebook-github-bot commented Jan 25, 2023

ltamasi Jan 24, 2023 •

edited

Loading

ltamasi Jan 24, 2023 •

edited

Loading

ltamasi Jan 24, 2023 •

edited

Loading