perf: seed component decls so cold loads avoid Θ(n²) tree walks - #34068
perf: seed component decls so cold loads avoid Θ(n²) tree walks#34068ljodea wants to merge 2 commits into
Conversation
Cold multi-loc component loads rematerialized the full loc→decl map once per uncached loc (Θ(n²) walks). Seed cacheable child component_decls after the first walk so subsequent loads are O(1) cache hits. Leave ComponentRootLoc and the unkeyed app-managed aggregate unseeded so app-managed discovery keeps rebuilding from a fresh listing.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Greptile SummaryImproves cold component loading by seeding cacheable declarations after the first tree walk.
Confidence Score: 5/5The PR appears safe to merge. The prior regression-test issue is fixed by requiring exactly one declaration-tree materialization, and no blocking failure remains.
|
| Filename | Overview |
|---|---|
| python_modules/dagster/dagster/components/core/component_tree.py | Adds declaration-cache lookup and seeds cacheable locations during the first full tree materialization while preserving fresh discovery locations. |
| python_modules/dagster/dagster_tests/components_tests/component_tree_tests/test_component_decl_tree_caching.py | Adds cold-load and cache-seeding regression coverage, including the exact single-materialization assertion requested by the prior review. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Cold component load] --> B{Declaration cached?}
B -->|Yes| C[Return cached declaration]
B -->|No| D[Materialize declaration tree]
D --> E[Seed cacheable child declarations]
E --> F[Load requested component]
F --> G[Later sibling loads use cache]
Reviews (2): Last reviewed commit: "test: assert single component-decl tree ..." | Re-trigger Greptile
| assert call_count < n_siblings, ( | ||
| f"_component_decl_tree was called {call_count} times for " | ||
| f"{n_siblings} sibling components (expected O(1) materializations, " | ||
| f"not once per component)" | ||
| ) |
There was a problem hiding this comment.
The assertion accepts up to 11 tree materializations for 12 siblings, so loading nearly every sibling through a separate tree walk still passes despite the test's O(1) contract. Assert the expected single materialization so a regression toward quadratic cold-load behavior is detected.
| assert call_count < n_siblings, ( | |
| f"_component_decl_tree was called {call_count} times for " | |
| f"{n_siblings} sibling components (expected O(1) materializations, " | |
| f"not once per component)" | |
| ) | |
| assert call_count == 1, ( | |
| f"_component_decl_tree was called {call_count} times for " | |
| f"{n_siblings} sibling components (expected one materialization)" | |
| ) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Valid — post-fix cold load materializes the tree once. Tightened to assert call_count == 1 in afe98d2.
Tighten the call-count check so near-quadratic cold loads cannot pass.
Summary
Cold multi-loc component loads rematerialized the full loc→decl map once per uncached loc, so loading n components did Θ(n²) tree walks.
_component_decl_tree()walk, seedcomponent_declinto the state tracker for cacheable locs (ComponentPathand keyed app-managed locs).ComponentRootLocand the unkeyed app-managed aggregate unseeded so app-managed discovery still rebuilds from a fresh listing.find_decl_at_pathchecks the state tracker first after a prior walk.Complexity
n = number of component locations in the project (YAML folders/files, Python defs modules, keyed app-managed IDs).
Note: The filesystem root decl was already cached (
_get_filesystem_decl). The cost was re-walking the in-memory decl graph and rebuilding the loc→decl dict for every uncached child load—not re-scanning the filesystem each time.Audit target
python_modules/dagster/dagster/components/coreTest plan
test_component_decl_tree_caching.pybuild_defsof 12 siblings must not call_component_decl_treeonce per siblingcomponent_declentriesruff checkon touched filesFollow-ups
None. No other deferred complexity findings from this audit of
components/core.