Skip to content

perf(l1): enable lz4 compression for trie and flat key-value tables#6530

Open
dicethedev wants to merge 3 commits intolambdaclass:mainfrom
dicethedev:perf/lz4-compression-state-tables
Open

perf(l1): enable lz4 compression for trie and flat key-value tables#6530
dicethedev wants to merge 3 commits intolambdaclass:mainfrom
dicethedev:perf/lz4-compression-state-tables

Conversation

@dicethedev
Copy link
Copy Markdown

Motivation
State tables (ACCOUNT_TRIE_NODES, STORAGE_TRIE_NODES, ACCOUNT_FLATKEYVALUE, STORAGE_FLATKEYVALUE) were previously uncompressed. On IO-bound nodes, reading fewer bytes from disk outweighs the decompression cost, LZ4 decompresses at ~4 GB/s, making it effectively free relative to disk latency.

Description
Adds the four state CFs to compressible_tables so they receive DBCompressionType::Lz4 alongside the existing block/receipt tables. No other logic changes.

Checklist

  • Updated STORE_SCHEMA_VERSION (crates/storage/lib.rs) if the PR includes breaking changes to the Store requiring a re-sync.

Closes #5939

@dicethedev dicethedev requested a review from a team as a code owner April 25, 2026 10:20
@dicethedev dicethedev changed the title Perf/lz4 compression state tables perf(l1): enable lz4 compression for trie and flat key-value tables Apr 25, 2026
@dicethedev dicethedev changed the title perf(l1): enable lz4 compression for trie and flat key-value tables perf(l1): enable lz4 compression for trie and flat key-value tables Apr 25, 2026
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 25, 2026

Greptile Summary

This PR adds ACCOUNT_TRIE_NODES, STORAGE_TRIE_NODES, ACCOUNT_FLATKEYVALUE, and STORAGE_FLATKEYVALUE to the compressible_tables array so they receive DBCompressionType::Lz4, matching the rationale that LZ4 decompression speed (~4 GB/s) easily offsets disk I/O savings on IO-bound nodes. Beyond what the description states, the PR also silently adds DataBlockIndexType::BinaryAndHash with hash_ratio=0.75 to every existing CF match arm, which is an unannounced scope expansion worth explicit review and benchmarking.

Confidence Score: 4/5

Safe to merge with minor concerns — LZ4 compression addition is correct; undocumented BinaryAndHash scope should be acknowledged.

Only P2 findings: the core compression change is straightforward and RocksDB handles it gracefully for existing data; the BinaryAndHash addition to pre-existing CFs is undocumented but not incorrect. No schema version bump is needed since RocksDB compression changes are backward compatible at the CF level.

crates/storage/backend/rocksdb.rs — review the full scope of BinaryAndHash changes across all CF arms.

Important Files Changed

Filename Overview
crates/storage/backend/rocksdb.rs Adds LZ4 compression for the four state CFs and also (without mention in the PR description) applies DataBlockIndexType::BinaryAndHash with hash_ratio=0.75 to all existing CF match arms; trailing whitespace on several added lines.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[RocksDBBackend::open] --> B{CF in compressible_tables?}
    B -- Yes --> C[DBCompressionType::Lz4]
    B -- No --> D[DBCompressionType::None]
    C --> E{match cf_name}
    D --> E
    E --> F[HEADERS / BODIES 32KB block]
    E --> G[CANONICAL_BLOCK_HASHES / BLOCK_NUMBERS 16KB + bloom + BinaryAndHash NEW]
    E --> H[ACCOUNT_TRIE_NODES / STORAGE_TRIE_NODES 16KB + bloom + BinaryAndHash NEW + Lz4 NEW]
    E --> I[ACCOUNT_FLATKEYVALUE / STORAGE_FLATKEYVALUE 16KB + bloom + BinaryAndHash NEW + Lz4 NEW]
    E --> J[ACCOUNT_CODES 32KB blob + BinaryAndHash NEW]
    E --> K[RECEIPTS 32KB + BinaryAndHash NEW]
    E --> L[default 16KB + BinaryAndHash NEW]
Loading

Comments Outside Diff (1)

  1. crates/storage/backend/rocksdb.rs, line 120-204 (link)

    P2 Undocumented scope: BinaryAndHash block index applied to all CFs

    The PR description states "No other logic changes," but set_data_block_index_type(BinaryAndHash) and set_data_block_hash_ratio(0.75) are added to all match arms — including pre-existing CFs (CANONICAL_BLOCK_HASHES, BLOCK_NUMBERS, ACCOUNT_CODES, RECEIPTS, and the default branch) — not only the four newly compressed state tables. At hash_ratio = 0.75, each 16 KB data block gets ~12 KB of hash-index overhead appended, increasing SST file sizes for those existing CFs. This broader change should be called out explicitly and benchmarked separately from the LZ4 compression addition.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: crates/storage/backend/rocksdb.rs
    Line: 120-204
    
    Comment:
    **Undocumented scope: `BinaryAndHash` block index applied to all CFs**
    
    The PR description states "No other logic changes," but `set_data_block_index_type(BinaryAndHash)` and `set_data_block_hash_ratio(0.75)` are added to _all_ `match` arms — including pre-existing CFs (`CANONICAL_BLOCK_HASHES`, `BLOCK_NUMBERS`, `ACCOUNT_CODES`, `RECEIPTS`, and the default branch) — not only the four newly compressed state tables. At `hash_ratio = 0.75`, each 16 KB data block gets ~12 KB of hash-index overhead appended, increasing SST file sizes for those existing CFs. This broader change should be called out explicitly and benchmarked separately from the LZ4 compression addition.
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: crates/storage/backend/rocksdb.rs
Line: 120-204

Comment:
**Undocumented scope: `BinaryAndHash` block index applied to all CFs**

The PR description states "No other logic changes," but `set_data_block_index_type(BinaryAndHash)` and `set_data_block_hash_ratio(0.75)` are added to _all_ `match` arms — including pre-existing CFs (`CANONICAL_BLOCK_HASHES`, `BLOCK_NUMBERS`, `ACCOUNT_CODES`, `RECEIPTS`, and the default branch) — not only the four newly compressed state tables. At `hash_ratio = 0.75`, each 16 KB data block gets ~12 KB of hash-index overhead appended, increasing SST file sizes for those existing CFs. This broader change should be called out explicitly and benchmarked separately from the LZ4 compression addition.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: crates/storage/backend/rocksdb.rs
Line: 128-129

Comment:
**Trailing whitespace on newly added lines**

Several of the newly added lines carry trailing spaces (e.g. lines 128, 129, 143, 144, 158, 159, 175, 188, 201). Most CI linters and `rustfmt` will flag these; cleaning them up avoids noisy diffs in future PRs.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "fix(l1): correct typo 10s24 → 1024" | Re-trigger Greptile

Comment thread crates/storage/backend/rocksdb.rs Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider LZ4 for State Tables (RocksDB)

1 participant