-
Notifications
You must be signed in to change notification settings - Fork 217
fix(l1): size the RocksDB block cache from available memory #7093
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.
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,7 +75,7 @@ const IN_MEMORY_COMMIT_THRESHOLD: usize = 10000; | |
| /// import only ever extend a single canonical chain (no competing forks to mis-commit). | ||
| pub const BATCH_COMMIT_THRESHOLD: usize = 4; | ||
|
|
||
| /// Default size in bytes of the RocksDB shared block cache: 12 GiB. | ||
| /// Ceiling on the RocksDB shared block cache: 12 GiB. | ||
| /// | ||
| /// This cache holds both data blocks AND the index/bloom-filter blocks for every | ||
| /// open SST file (because we enable `cache_index_and_filter_blocks`), so its size | ||
|
|
@@ -84,7 +84,85 @@ pub const BATCH_COMMIT_THRESHOLD: usize = 4; | |
| /// synced mainnet node (32 GiB cap) found 8-16 GiB all keep up with head-following, | ||
| /// with larger giving no gain (the OS page cache backstops the uncompressed state | ||
| /// CFs) and ~8 GiB the floor where the filter set starts to thrash. | ||
| pub const DEFAULT_ROCKSDB_BLOCK_CACHE_SIZE_BYTES: usize = 12 * 1024 * 1024 * 1024; | ||
| pub const MAX_ROCKSDB_BLOCK_CACHE_SIZE_BYTES: usize = 12 * 1024 * 1024 * 1024; | ||
|
|
||
| /// Floor on the RocksDB shared block cache: 512 MiB. Below this the per-SST | ||
| /// index and filter blocks no longer stay resident and every trie read pays an | ||
| /// extra disk seek, which is worse than the memory it saves. Applied even when | ||
| /// it exceeds [`ROCKSDB_BLOCK_CACHE_MEMORY_PERCENT`] of a very small host. | ||
| pub const MIN_ROCKSDB_BLOCK_CACHE_SIZE_BYTES: usize = 512 * 1024 * 1024; | ||
|
|
||
| /// Share of the host's (or cgroup's) memory the block cache may claim by default. | ||
| /// | ||
| /// The rest has to cover the in-memory trie-layer backlog, block execution, the | ||
| /// mempool, peer buffers and allocator slack, so the cache cannot have most of the | ||
| /// machine. At 40% a 32 GiB host lands on the 12 GiB ceiling and a 16 GiB host gets | ||
| /// ~6.4 GiB — under the ~8 GiB thrash floor, but that is the memory-constrained | ||
| /// tradeoff the ceiling's docs describe, and it leaves the node room not to be | ||
| /// OOM-killed. | ||
| pub const ROCKSDB_BLOCK_CACHE_MEMORY_PERCENT: usize = 40; | ||
|
|
||
| /// Default RocksDB shared block cache size, derived from the memory this process | ||
| /// is actually allowed to use. | ||
| /// | ||
| /// A fixed default cannot serve both a 64 GiB validator and a 16 GiB CI runner: the | ||
| /// ceiling alone is 71% of a 16 GiB host, which leaves no headroom for the rest of | ||
| /// the node. Sizes to [`ROCKSDB_BLOCK_CACHE_MEMORY_PERCENT`] of the detected limit, | ||
| /// clamped to [`MIN_ROCKSDB_BLOCK_CACHE_SIZE_BYTES`] ..= | ||
| /// [`MAX_ROCKSDB_BLOCK_CACHE_SIZE_BYTES`]. Falls back to the ceiling when the limit | ||
| /// cannot be detected, preserving the previous behavior. | ||
| pub fn default_rocksdb_block_cache_size() -> usize { | ||
| rocksdb_block_cache_size_for(host_memory_limit_bytes()) | ||
| } | ||
|
|
||
| /// Pure part of [`default_rocksdb_block_cache_size`]: the clamp, with the detected | ||
| /// memory limit supplied by the caller. `None` means detection failed. | ||
| pub fn rocksdb_block_cache_size_for(memory_limit: Option<usize>) -> usize { | ||
| let Some(limit) = memory_limit else { | ||
| return MAX_ROCKSDB_BLOCK_CACHE_SIZE_BYTES; | ||
| }; | ||
| (limit / 100 * ROCKSDB_BLOCK_CACHE_MEMORY_PERCENT).clamp( | ||
| MIN_ROCKSDB_BLOCK_CACHE_SIZE_BYTES, | ||
| MAX_ROCKSDB_BLOCK_CACHE_SIZE_BYTES, | ||
| ) | ||
| } | ||
|
|
||
| /// Memory this process may use: the smaller of the host's physical memory and the | ||
| /// cgroup memory limit, so a container gets sized against its own limit rather than | ||
| /// the machine it happens to run on. `None` when nothing could be read (non-Linux, | ||
| /// or an unreadable/absent `/proc`), which callers treat as "unknown". | ||
| fn host_memory_limit_bytes() -> Option<usize> { | ||
| [physical_memory_bytes(), cgroup_memory_limit_bytes()] | ||
| .into_iter() | ||
| .flatten() | ||
| .min() | ||
| } | ||
|
|
||
| /// `MemTotal` from `/proc/meminfo`, in bytes. | ||
| fn physical_memory_bytes() -> Option<usize> { | ||
| let meminfo = std::fs::read_to_string("/proc/meminfo").ok()?; | ||
| let kib: usize = meminfo | ||
| .lines() | ||
| .find_map(|line| line.strip_prefix("MemTotal:"))? | ||
| .split_whitespace() | ||
| .next()? | ||
| .parse() | ||
| .ok()?; | ||
| kib.checked_mul(1024) | ||
| } | ||
|
|
||
| /// The cgroup memory limit, in bytes: `memory.max` under cgroup v2, falling back to | ||
| /// `memory.limit_in_bytes` under v1. Both report "no limit" out of band — v2 with the | ||
| /// literal `max`, v1 with a sentinel near `u64::MAX` — which yields `None` here (v1's | ||
| /// sentinel simply exceeds any real `MemTotal`, so the `min` above discards it). | ||
| fn cgroup_memory_limit_bytes() -> Option<usize> { | ||
| [ | ||
| "/sys/fs/cgroup/memory.max", | ||
| "/sys/fs/cgroup/memory/memory.limit_in_bytes", | ||
| ] | ||
| .into_iter() | ||
| .find_map(|path| std::fs::read_to_string(path).ok()?.trim().parse().ok()) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If ethrex runs in a nested cgroup with a memory limit, these fixed mount-root paths read the root cgroup rather than the process's cgroup. The limit is therefore ignored and startup can select the 12 GiB cache ceiling inside a smaller allocation, causing the node to be OOM-killed. Knowledge Base Used: Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/storage/store.rs
Line: 158-164
Comment:
**Nested cgroup limit is ignored**
If ethrex runs in a nested cgroup with a memory limit, these fixed mount-root paths read the root cgroup rather than the process's cgroup. The limit is therefore ignored and startup can select the 12 GiB cache ceiling inside a smaller allocation, causing the node to be OOM-killed.
**Knowledge Base Used:**
- [Storage Layer](https://app.greptile.com/lambdaclass/-/custom-context/knowledge-base/lambdaclass/ethrex/-/docs/storage-layer.md)
- [CLI Entrypoint and Node Startup](https://app.greptile.com/lambdaclass/-/custom-context/knowledge-base/lambdaclass/ethrex/-/docs/cli-entrypoint.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly. |
||
| } | ||
|
|
||
| /// Tunable configuration for [`Store::new_with_config`] and related constructors. | ||
| /// | ||
|
|
@@ -107,7 +185,7 @@ pub struct StoreConfig { | |
| impl Default for StoreConfig { | ||
| fn default() -> Self { | ||
| Self { | ||
| rocksdb_block_cache_size: DEFAULT_ROCKSDB_BLOCK_CACHE_SIZE_BYTES, | ||
| rocksdb_block_cache_size: default_rocksdb_block_cache_size(), | ||
| persist_channel_capacity: DEFAULT_PERSIST_CHANNEL_CAPACITY, | ||
| } | ||
| } | ||
|
|
@@ -5018,8 +5096,8 @@ pub fn read_chain_id_from_db(path: &Path) -> Option<u64> { | |
| #[cfg(feature = "rocksdb")] | ||
| { | ||
| // The cache size is irrelevant for this one-shot chain-id read (the LRU | ||
| // is sized as a ceiling, not pre-allocated), so we use the default. | ||
| let backend = match RocksDBBackend::open(path, DEFAULT_ROCKSDB_BLOCK_CACHE_SIZE_BYTES) { | ||
| // is sized as a ceiling, not pre-allocated), so we use the ceiling. | ||
| let backend = match RocksDBBackend::open(path, MAX_ROCKSDB_BLOCK_CACHE_SIZE_BYTES) { | ||
| Ok(backend) => backend, | ||
| Err(e) => { | ||
| warn!("Failed to open RocksDB at {path:?} to read chain ID: {e}"); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| mod deferred_persistence_tests; | ||
| mod fcu_race_tests; | ||
| mod rocksdb_block_cache_tests; | ||
| mod store_tests; | ||
| mod trie_db_tests; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| //! Sizing of the RocksDB shared block cache default. | ||
|
|
||
| use ethrex_storage::{ | ||
| MAX_ROCKSDB_BLOCK_CACHE_SIZE_BYTES, MIN_ROCKSDB_BLOCK_CACHE_SIZE_BYTES, | ||
| ROCKSDB_BLOCK_CACHE_MEMORY_PERCENT, default_rocksdb_block_cache_size, | ||
| rocksdb_block_cache_size_for, | ||
| }; | ||
|
|
||
| const GIB: usize = 1024 * 1024 * 1024; | ||
|
|
||
| /// An undetectable memory limit SHALL fall back to the ceiling, preserving the | ||
| /// behavior from before the default became memory-aware. | ||
| #[test] | ||
| fn undetected_memory_limit_falls_back_to_the_ceiling() { | ||
| assert_eq!( | ||
| rocksdb_block_cache_size_for(None), | ||
| MAX_ROCKSDB_BLOCK_CACHE_SIZE_BYTES | ||
| ); | ||
| } | ||
|
|
||
| /// Hosts large enough that the percentage exceeds the ceiling SHALL get the ceiling, | ||
| /// so a big machine keeps the tuned 12 GiB rather than scaling without bound. | ||
| #[test] | ||
| fn large_hosts_are_capped_at_the_ceiling() { | ||
| for limit in [32 * GIB, 64 * GIB, 1024 * GIB] { | ||
| assert_eq!( | ||
| rocksdb_block_cache_size_for(Some(limit)), | ||
| MAX_ROCKSDB_BLOCK_CACHE_SIZE_BYTES, | ||
| "{limit} bytes must clamp to the ceiling" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /// The case this sizing exists for: on a 16 GiB host the flat 12 GiB default was 71% of | ||
| /// the machine, leaving no headroom for trie layers, execution and the mempool. The | ||
| /// memory-aware default SHALL leave the majority of such a host to the rest of the node. | ||
| #[test] | ||
| fn memory_constrained_host_keeps_headroom() { | ||
| let limit = 16 * GIB; | ||
| let cache = rocksdb_block_cache_size_for(Some(limit)); | ||
|
|
||
| assert_eq!(cache, limit / 100 * ROCKSDB_BLOCK_CACHE_MEMORY_PERCENT); | ||
| assert!( | ||
| cache < MAX_ROCKSDB_BLOCK_CACHE_SIZE_BYTES, | ||
| "a 16 GiB host must not be handed the ceiling" | ||
| ); | ||
| assert!( | ||
| limit - cache > limit / 2, | ||
| "more than half of a 16 GiB host must stay available to the node" | ||
| ); | ||
| } | ||
|
|
||
| /// Tiny hosts SHALL still get the floor: below it the per-SST index and filter blocks | ||
| /// stop staying resident and every trie read pays an extra seek. | ||
| #[test] | ||
| fn tiny_hosts_get_the_floor() { | ||
| assert_eq!( | ||
| rocksdb_block_cache_size_for(Some(GIB)), | ||
| MIN_ROCKSDB_BLOCK_CACHE_SIZE_BYTES | ||
| ); | ||
| assert_eq!( | ||
| rocksdb_block_cache_size_for(Some(0)), | ||
| MIN_ROCKSDB_BLOCK_CACHE_SIZE_BYTES | ||
| ); | ||
| } | ||
|
|
||
| /// Whatever this machine reports, the resolved default SHALL land inside the clamp. | ||
| #[test] | ||
| fn resolved_default_is_within_the_clamp() { | ||
| let cache = default_rocksdb_block_cache_size(); | ||
| assert!( | ||
| (MIN_ROCKSDB_BLOCK_CACHE_SIZE_BYTES..=MAX_ROCKSDB_BLOCK_CACHE_SIZE_BYTES).contains(&cache), | ||
| "resolved default {cache} outside the clamp" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing logs the detected limit or the size this returns, and that is the one thing the PR'''s own motivation argues for.
The old value was a constant: wrong on small hosts, but knowable from the source. This replaces it with a number derived from
/proc/meminfo, cgroup v2memory.max, or cgroup v1memory.limit_in_bytes, whichever is smallest — andNonefrom all of them silently restores the old 12 GiB. So the cache size is now environment-dependent and invisible, on a code path that exists because a mis-sized cache was only discovered via an OOM kill.Concretely, an operator debugging memory today cannot answer: was a cgroup limit detected at all, or did we fall back to physical memory? Did detection fail entirely and hand back 12 GiB? Was the result clamped? The clamp matters because it is silent in both directions — below ~1.3 GiB of detected limit,
MIN_ROCKSDB_BLOCK_CACHE_SIZE_BYTES(512 MiB) wins and the cache is more than 40% of the limit.One line at startup would close it:
Worth noting the blast radius is small, which is a point in the PR'''s favour and not stated: with a 40% factor clamped to 12 GiB, the default only moves below ~30 GiB of detected memory.
docs/getting-started/hardware_requirements.mdputs the RAM minimum at 32 GB for every network, so on any spec-compliant host this is a no-op (32 GB -> 12.8 GiB -> clamped to 12 GiB, unchanged). It changes behaviour only for under-spec machines and containers — which is exactly the intent, and worth saying explicitly so reviewers know a spec-compliant deployment sees no change.