feat(test-fill,test-specs): add t8n call caching to test specs#2084
feat(test-fill,test-specs): add t8n call caching to test specs#2084marioevz merged 30 commits intoethereum:forks/amsterdamfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #2084 +/- ##
================================================
Coverage 86.11% 86.11%
================================================
Files 599 599
Lines 39472 39472
Branches 3780 3780
================================================
Hits 33992 33992
Misses 4852 4852
Partials 628 628
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
packages/testing/src/execution_testing/cli/pytest_commands/plugins/filler/filler.py
Show resolved
Hide resolved
marioevz
left a comment
There was a problem hiding this comment.
Running the latest version I see that we are adding @t8n-cache-<md5-hash> to the names of all tests for some reason. This seems incorrect to me, so I'd like to give this a proper re-review.
This is to ensure that all parametrized test formats (state_test, blockchain_test, blockchain_test_engine) for a single test case get distributed to the same xdist worker to ensure they use the same per-worker cache; the cache is not global across workers. |
a294da7 to
0b598ec
Compare
0b598ec to
bfae73d
Compare
bfae73d to
069d10c
Compare
|
Just a general comment (haven't looked at the code yet), but are the caches per-thread/per-process or global? If they aren't global, you might need to load group them to get the biggest benefit. |
f57d1d5 to
bebe55a
Compare
The caches are per xdist process, not global. So yes, to make this work, |
- Enable pytest-xdist loadgroup distribution mode by default. - Required for xdist_group markers to control worker assignment.
- Add strip_fixture_format_from_nodeid() to extract base nodeid. - Add get_all_fixture_format_names() for format name lookup. - Used to ensure related fixture formats share cache keys.
- Add T8nOutputCache LRU cache class for storing t8n outputs.
- Add t8n_output_cache field to FillingSession.
- Add xdist_group markers during collection for --dist=loadgroup.
- Use t8n-cache-{hash} prefix to distinguish from user-defined groups.
- Strip cache-specific @t8n-cache-* suffix from nodeids in TestInfo.
- Add cache key helpers to BaseTest (_get_base_nodeid, _get_t8n_cache_key). - Add _get_filling_session() to access cache from test instances. - Cache t8n outputs in _generate_block_data() for reuse across formats. - Skip caching for engine_x and engine_sync variants (different execution).
- Test T8nOutputCache LRU behavior, eviction, and hit/miss tracking. - Test strip_fixture_format_from_nodeid for various nodeid patterns. - Test get_all_fixture_format_names ordering and contents. - Test cache key consistency across fixture format variants. - Test _strip_xdist_group_suffix preserves non-cache group markers.
Add tests to verify that test items are sorted during collection to ensure deterministic cache hits. The tests demonstrate: - Sorting groups related fixture formats by base nodeid. - Without xdist, items are correctly sorted. - With xdist, items are NOT sorted (BUG causing high variance). - Expected vs actual behavior comparison. The xfail test `test_xdist_sorting_required_for_cache_hits` asserts the correct behavior (sorting with xdist) and fails until the fix is applied.
- Add helper methods to TransitionToolCacheStats for serialization. - Initialize aggregated stats on xdist controller in pytest_configure. - Send worker stats via workeroutput in fixture teardown. - Add pytest_testnodedown hook to aggregate stats from workers. - Update pytest_terminal_summary to display aggregated stats.
- Clear `_cache` in `remove_cache()` to prevent stale data leakage. - Tests without `transition_tool_cache_key` (e.g., state_test) could previously retrieve cached results from prior tests via matching `call_counter` subkeys.
Sort test items by (is_slow, base_nodeid, nodeid) to optimize execution: - Slow tests first (LPT scheduling for xdist load balance). - Related fixture formats grouped together (cache locality). - Deterministic order within groups. If ANY fixture format variant of a test is marked slow, ALL variants are treated as slow to keep them grouped together for cache hits. Reuses the base_nodeid cache for xdist marker generation to avoid redundant strip_fixture_format_from_node calls.
BlockchainEngineXFixture and BlockchainEngineSyncFixture had can_use_cache=False which was dead code (never checked anywhere). Replace with transition_tool_cache_key="" which is the actual mechanism that controls caching — empty string means no caching.
For StateTest specs with --generate-all-formats, the _from_state_test label suffixes cause alphabetical sort to interleave cacheable and non-cacheable formats: blockchain_test_engine_from_state_test (cacheable) → blockchain_test_engine_x_from_state_test (non-cacheable, clears cache) → blockchain_test_from_state_test (cacheable, but cache is gone). Add has_cache_key to the sort key so cacheable formats cluster together within each base nodeid group, ensuring the second cacheable format hits the warm cache before any non-cacheable format clears it.
node_id_for_entropy strips fixture format and fork names from the node ID before hashing it for deterministic address generation. However, it did not strip the xdist @group_name suffix (e.g., @t8n-cache-abc12345), causing different addresses when running with vs without xdist workers. Strip the suffix so addresses are deterministic regardless of whether xdist is active.
Replace the raw hit/miss counts with an efficiency metric where 100% means all tests that could have hit the cache did hit it. Track unique cache keys to compute expected hits (total cacheable - unique keys). Also filter subkey stats to only count cacheable tests, eliminating phantom misses from non-cacheable tests that still interact with the OutputCache after remove_cache(). Before: T8n cache: key_hits=6, key_misses=6 (50.0%), subkey_hits=6, subkey_misses=18 (25.0%) After: T8n cache: 100% hit rate (6/6 expected), 6 t8n calls saved
Pydantic's ModelMetaclass caches __init__ wrappers for dynamically created classes. When pytester runs multiple fill sessions in-process, cached wrappers from an earlier session re-invoke __init__ re-entrantly in later sessions, causing generate() to run twice per test and doubling the opcode count. - Switch fill tests to runpytest_subprocess() for process isolation. - Normal `fill` runs are unaffected (each invocation is a fresh process).
The rebase introduced a second pytest_testnodedown definition for cache stats aggregation, shadowing the existing timing logs hook. Extract the cache stats logic into _aggregate_cache_stats() and call it from the single hook.
Remove dead code that was never called: _get_base_nodeid(), _get_t8n_cache_key(), _get_filling_session() from BaseTest and remove_opcode_count() from TransitionTool. Also remove the now-unused strip_fixture_format_from_node import, TYPE_CHECKING import, and guarded FillingSession import block from base.py.
Add a clear() method to OutputCache to encapsulate clearing the
internal _cache dict and resetting the key, instead of having
TransitionTool.remove_cache() reach into private members directly.
Also fix the set_cache docstring ("LRU behavior" → "single-key
eviction"), fix the cached_result truthiness check to use
`is not None` for defensive correctness, and fix the node()
docstring to say "pytest node" instead of "node ID".
The @t8n-cache-* xdist group suffix was leaking into the test_id and group_salt stored in pre-alloc groups, causing fixture output to differ from runs without xdist. Use _strip_xdist_group_suffix() (already used by node_to_test_info) on both the group_salt fallback and the test_id passed to add_test_pre.
Test set_key eviction, get/set round-trip, hit/miss counter accuracy, clear() behavior, and state across key changes. Uses sentinel objects as lightweight stand-ins for TransitionToolOutput.
bebe55a to
eb2d1be
Compare
|
I ran |
|
There's a further possible optimization for the |
marioevz
left a comment
There was a problem hiding this comment.
Looks great, thanks for implementing this!
|
I'm fixing the merge conflict in a bit and then merging 👍 |
🗒️ Description
Add an in-memory output cache for transition tool results during fixture generation. When multiple fixture formats share the same t8n inputs (e.g.,
blockchain_testandblockchain_test_engine), the cache eliminates redundant t8n calls.Changes
OutputCacheclass that stores t8n results per test, keyed by call counter.strip_fixture_format_from_node()to derive consistent cache keys across fixture formats.TransitionTool.evaluate(), used byBlockchainTest.generate_block_data().xdist_groupmarkers to keep related formats on the same worker.Performance
The
t8n-call-cache-specsbranch reduces thepy3fixture fill step duration by 31% compared to other recent PRs (mean 18:58 vs 27:35, n=3 vs n=15).Methodology
The analysis compares the timing of the "Run py3 tests" step from the
py3job from this PR with other PRs.py3CI job (GitHub Actionstest.yamlworkflow).Notes on the Achieved vs Expected Gain
The numbers below regarding % of t8n overhead shouldn't be taken too seriously, but it was fun to try and derive these 🙂
The
py3CI job generates 2 fixture formats forBlockchainTestspecs and up to 3 forStateTestspecs (blockchain_test_engine_xis not yet part ofpy3). The cache stores t8n output fromblockchain_testand replays it forblockchain_test_engine, which shares the sametransition_tool_cache_key.Test item counts (
--collect-only,py3tox environment)state_testblockchain_test(native)blockchain_test_engine(native)blockchain_test_from_state_testblockchain_test_engine_from_state_testNative blockchain counts are estimated as an equal split of the 23,800 native blockchain items (65,979 total blockchain minus 23,414 and 18,765 derived-from-state-test items).
Not all
StateTestspecs generateblockchain_test_engine: 4,649 state tests produceblockchain_testbut not the engine variant (due to tests for forks that predate the engine format).Expected vs observed savings
The remaining ~9% of fill time is non-t8n overhead: fixture serialization, pre-allocation, and pytest machinery.
🔗 Related Issues or PRs
N/A.
✅ Checklist
toxchecks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx tox -e statictype(scope):.mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.Cute Animal Picture