Skip to content

fix:block_cache_size #3169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
chejinge wants to merge 2 commits into from
Closed

fix:block_cache_size #3169

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e179629
fix:block_cache_size
brother-jin Sep 24, 2025
878e918
fix:block_cache_size
brother-jin Sep 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions src/pika_server.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1479,12 +1479,21 @@ void PikaServer::InitStorageOptions() {
storage_options_.table_options.pin_l0_filter_and_index_blocks_in_cache =
g_pika_conf->pin_l0_filter_and_index_blocks_in_cache();

if (storage_options_.block_cache_size == 0) {
storage_options_.table_options.no_block_cache = true;
} else if (storage_options_.share_block_cache) {
storage_options_.table_options.block_cache =
rocksdb::NewLRUCache(storage_options_.block_cache_size, static_cast<int>(g_pika_conf->num_shard_bits()));
}
if (storage_options_.block_cache_size == 0) {
storage_options_.table_options.no_block_cache = true;
storage_options_.table_options.block_cache.reset();
} else if (storage_options_.share_block_cache) {
// 共享模式：直接复用已创建好的全局 cache（PikaServer 成员）
storage_options_.table_options.no_block_cache = false;
assert(share_block_cache_ && "shared_block_cache_ must be initialized before InitStorageOptions()");
storage_options_.table_options.block_cache = shared_block_cache_;
} else {
Comment on lines +1485 to +1493
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Shared cache branch never attaches the shared cache

When share_block_cache is enabled we still leave block_cache as nullptr. RocksDB will silently fall back to its default per-instance cache instead of reusing the intended global cache, so the “shared” mode never actually shares anything. The guard also re-checks the config flag instead of validating that shared_block_cache_ has been initialized.

Wire the shared cache through (and assert it exists) before we flip no_block_cache off:

-    } else if (storage_options_.share_block_cache) {
-        if (!g_pika_conf->share_block_cache()) {
-            assert(false && "shared_block_cache_ must be initialized before InitStorageOptions()");
-        }
-
-        storage_options_.table_options.no_block_cache = false;
-
-        storage_options_.table_options.block_cache = 0;
+    } else if (storage_options_.share_block_cache) {
+        storage_options_.table_options.no_block_cache = false;
+        assert(shared_block_cache_ && "shared_block_cache_ must be initialized before InitStorageOptions()");
+        storage_options_.table_options.block_cache = shared_block_cache_;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
} else if (storage_options_.share_block_cache) {
if (!g_pika_conf->share_block_cache()) {
assert(false && "shared_block_cache_ must be initialized before InitStorageOptions()");
}
if (storage_options_.block_cache_size == 0) {
storage_options_.table_options.no_block_cache = true;
} else if (storage_options_.share_block_cache) {
storage_options_.table_options.block_cache =
rocksdb::NewLRUCache(storage_options_.block_cache_size, static_cast<int>(g_pika_conf->num_shard_bits()));
}
storage_options_.table_options.no_block_cache = false;
storage_options_.table_options.block_cache = 0;
} else {
} else if (storage_options_.share_block_cache) {
storage_options_.table_options.no_block_cache = false;
assert(shared_block_cache_ && "shared_block_cache_ must be initialized before InitStorageOptions()");
storage_options_.table_options.block_cache = shared_block_cache_;
} else {
🤖 Prompt for AI Agents 
In src/pika_server.cc around lines 1485-1493, the shared-cache branch flips
table_options.no_block_cache to false but leaves table_options.block_cache as
nullptr and re-checks g_pika_conf instead of validating the actual
shared_block_cache_ object; update this branch to assert that
shared_block_cache_ is initialized (not the config flag) and assign the shared
cache to storage_options_.table_options.block_cache before clearing
no_block_cache so RocksDB uses the intended global cache.

// 非共享模式：为当前 CF/实例单独创建
storage_options_.table_options.no_block_cache = false;
storage_options_.table_options.block_cache =
rocksdb::NewLRUCache(storage_options_.block_cache_size,
static_cast<int>(g_pika_conf->num_shard_bits()));
}
Comment on lines +1481 to +1498
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

🧩 Analysis chain

Verify shared_block_cache_ initialization order.

The code assumes shared_block_cache_ is initialized before InitStorageOptions() is called. Please verify that the shared cache is properly initialized in the constructor or startup sequence before this method is invoked.

🏁 Script executed:

#!/bin/bash
# Search for shared_block_cache_ initialization in the codebase
rg -n "shared_block_cache_.*=" --type cpp -A 3 -B 3

# Search for InitStorageOptions calls to understand initialization order
rg -n "InitStorageOptions" --type cpp -A 3 -B 3

# Look for shared_block_cache_ declaration
ast-grep --pattern 'shared_ptr<$_> shared_block_cache_'

Length of output: 1539

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Searching for share_block_cache_ / shared_block_cache_ occurrences..."
rg -n "share_block_cache_|shared_block_cache_" -S || true

echo
echo "Show declaration area in include/pika_server.h (lines 1-260):"
sed -n '1,260p' include/pika_server.h || true

echo
echo "Show top of src/pika_server.cc (lines 1-140) to see constructor/init order:"
sed -n '1,140p' src/pika_server.cc || true

echo
echo "Search for places that create or assign a block cache (NewLRUCache or MakeLRUCache or Assign to *_block_cache):"
rg -n "NewLRUCache|MakeLRUCache|_block_cache" -S || true

echo
echo "Search for functions/methods that might initialize a shared cache (InitShared|InitBlockCache|CreateSharedCache):"
rg -n "Init.*Block|Init.*Cache|Create.*Cache|shared.*cache" -S || true

Length of output: 126768

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Search for 'shared_block_cache' variants across the repo:"
rg -n "shared_block_cache|share_block_cache_|shared_block_cache_" -S || true

echo
echo "Show remainder of include/pika_server.h to find member declarations (lines 260-999):"
sed -n '260,999p' include/pika_server.h || true

echo
echo "Search for any member declarations referencing rocksdb::Cache or Cache pointer in headers:"
rg -n "rocksdb::Cache|std::shared_ptr<.*Cache>" -S || true

echo
echo "Show PikaServer private members area by locating closing of class (search for 'private:' within header):"
rg -n "class PikaServer|private:" include/pika_server.h -n -n -S || true
sed -n '420,999p' include/pika_server.h || true

Length of output: 18930

Fix shared_block_cache_ initialization and correct the assert typo.

InitStorageOptions() is called from the constructor (src/pika_server.cc:57). Inside InitStorageOptions (src/pika_server.cc:1426-1490) you assign storage_options_.table_options.block_cache = shared_block_cache_ but there is no observable declaration/initialization of shared_block_cache_ before that call, and the assert checks share_block_cache_ (typo). Initialize shared_block_cache_ before InitStorageOptions() (or move the InitStorageOptions() call) and change the assert to assert(shared_block_cache_ && ...) or otherwise guard the assignment.

🤖 Prompt for AI Agents 
In src/pika_server.cc around lines 1482-1496, InitStorageOptions() assigns
storage_options_.table_options.block_cache = shared_block_cache_ but
shared_block_cache_ is not guaranteed initialized and the assert erroneously
checks share_block_cache_; fix by correcting the assert to
assert(shared_block_cache_ && "shared_block_cache_ must be initialized before
InitStorageOptions()"), and ensure shared_block_cache_ is created/initialized
before InitStorageOptions() is called (either initialize it in the constructor
prior to calling InitStorageOptions() or move the InitStorageOptions() call
after shared_block_cache_ creation); alternatively add a runtime guard so you
only assign shared_block_cache_ when it is non-null to avoid dereferencing an
uninitialized pointer.

storage_options_.options.rate_limiter =
std::shared_ptr<rocksdb::RateLimiter>(
rocksdb::NewGenericRateLimiter(
Expand Down
Loading