chore: remove MARF trie-node caching#7371
Open
cylewitruk-stacks wants to merge 4 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the (currently unused) MARF trie-node caching plumbing and updates the surrounding configuration, storage internals, and test harness to match, with the goal of reducing long-running test time in CI/local runs.
Changes:
- Remove MARF trie-node cache strategy from
MARFOpenOptsand fromNodeConfig’s effective MARF options. - Replace the old cache implementation with a smaller
BlockHashCachethat only caches block-hash/block-ID lookups. - Update MARF-related tests and docs to use the new
MARFOpenOpts::new(hash_mode, external_blobs)API and new test option templates.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| stackslib/src/config/mod.rs | Removes marf_cache_strategy from runtime node config and MARF opts; keeps config-file field with deprecation warning. |
| stackslib/src/clarity_vm/tests/ephemeral.rs | Updates ephemeral MARF initialization to new MARFOpenOpts::new signature. |
| stackslib/src/clarity_vm/database/marf.rs | Updates in-memory MARF open options to new API. |
| stackslib/src/chainstate/stacks/index/test/trie.rs | Switches tests to updated rstest template (tpl_all_opts). |
| stackslib/src/chainstate/stacks/index/test/squash.rs | Updates squash tests to new MARFOpenOpts::new signature. |
| stackslib/src/chainstate/stacks/index/test/mod.rs | Removes cache-test module and simplifies MARF option matrices/templates. |
| stackslib/src/chainstate/stacks/index/test/marf.rs | Updates loops over MARF options to the new ALL_OPTS. |
| stackslib/src/chainstate/stacks/index/test/marf_regression.rs | Removes cache-strategy dimension from regression naming/coverage. |
| stackslib/src/chainstate/stacks/index/test/marf_perfs.rs | Updates perf tests to iterate updated MARF option list; log formatting cleanup. |
| stackslib/src/chainstate/stacks/index/test/file.rs | Updates file storage tests to new MARFOpenOpts::new signature. |
| stackslib/src/chainstate/stacks/index/storage.rs | Removes node/hash caching code paths; uses BlockHashCache for block-hash/block-ID caching only. |
| stackslib/src/chainstate/stacks/index/squash.rs | Updates squash open options to new MARF opts API. |
| stackslib/src/chainstate/stacks/index/marf.rs | Removes cache strategy field from MARFOpenOpts and updates call sites. |
| stackslib/src/chainstate/stacks/index/cache.rs | Replaces trie-node cache implementation with BlockHashCache. |
| stackslib/src/chainstate/stacks/db/snapshot/tests/clarity.rs | Updates snapshot squash/open calls to new MARFOpenOpts::new signature. |
| stackslib/src/chainstate/stacks/db/snapshot/fork_storage.rs | Updates readonly MARF open options to new signature. |
| stackslib/src/chainstate/stacks/db/snapshot/clarity.rs | Updates clarity readonly MARF open options to new signature. |
| docs/profiling.md | Updates debug-log example for config printing (needs follow-up for accuracy). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Coverage Report for CI Build 28520872114Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.3%) to 86.015%Details
Uncovered Changes
Coverage Regressions9151 previously-covered lines in 136 files lost coverage.
Coverage Stats
💛 - Coveralls |
| /// Instantiate the default strategy. This can be taken from the `STACKS_MARF_CACHE_STRATEGY` | ||
| /// environ, or failing that, it will use a no-op strategy. | ||
| pub fn default() -> TrieCache<T> { | ||
| if let Ok(strategy) = std::env::var("STACKS_MARF_CACHE_STRATEGY") { |
Contributor
There was a problem hiding this comment.
nit: maybe we should add in the changelog fragment also the removal of the env var?
| Ok(node_hash) | ||
| } | ||
| let node_hash = self.inner_read_persisted_node_hash(block_id, ptr)?; | ||
| self.bench.read_node_hash_finish(false); |
Contributor
There was a problem hiding this comment.
now that we don't have anymore the cache, maybe we can simplify read_node_hash_finish by removing the bool parameter (and the extra logic inside the function)
| let (node_inst, node_hash) = | ||
| self.inner_read_patched_persisted_nodetype(id, clear_ptr, read_hash)?; | ||
|
|
||
| self.bench.read_nodetype_finish(false); |
Contributor
There was a problem hiding this comment.
same comment as per read_node_hash_finish, but seems like that parameter was dead code even before this change
| /// The batch size is intentionally set above 10.000 to force batched insertion | ||
| /// and exercise the `eta` batching logic. | ||
| /// For all configurations, the resulting root hash must remain stable. | ||
| #[rstest] |
Contributor
There was a problem hiding this comment.
not a strong opinion, but maybe we can keep this one to test the 10k batched insertion?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removes the unused trie node cache (which is always defaulted as "noop" today) and related tests.
This was previously agreed within the team -- prioritizing now as the removal of the unnecessary caching tests removes a number of long-running tests run both in CI and locally.