Skip revisiting cleared dependencies when validating resolution paths - #143
Conversation
assert_valid_resolution_path detects cycles by walking the dependency graph depth-first, but it enumerates every distinct root-to-leaf path and never records that a node's subtree already came back clean. In a graph where many injectables share dependencies -- the normal shape once a few services sit on top of a common core -- each shared subgraph is re-walked once per incoming path, so the visit count grows with the number of paths through the graph rather than with its size. Container creation pays for this on every startup. Thread a set of cleared object ids through the walk and return early for any dependency already in it, adding to it only once the whole subtree has returned without raising. Skipping a cleared dependency is sound: were it part of a cycle, the walk would have come back to it while it was still on the current path, and that check -- along with the resulting error message -- is unchanged. Marking happens after the recursion, not before, so a node that is still on the stack is never treated as cleared. The set is created in validate_registry and lives for exactly one run. It is deliberately not module level: two registries describe two different graphs, and a node cleared in one says nothing about the other. On a synthetic graph of 316 injectables (fan-out 3, depth 9) the path check goes from 1,637,664 visits to 2,412, and validate_registry from 2612 ms to 2.6 ms. On a real ~320-type application container it goes from 418,891 visits to 550, cutting 1.40 s off a 1.97 s container build. The cycle validation this changes was added in maldoinc#83 for maldoinc#80.
maldoinc
left a comment
maldoinc
left a comment
There was a problem hiding this comment.
Overall MR looks good, just a tiny naming nit.
Unrelated observation
We can probably refactor the current graph validation to not do two jobs at once.
perhaps there can be a separate step after extending but before validation runs to perform any cleanup on unknown objects. Feel free to send an additional PR for this.
| # Dependencies are shared between injectables, so the same subtree is reachable via many paths. | ||
| # Remember the ones already known to be cycle-free to avoid walking them again. | ||
| # Only valid for this run as a different registry describes a different graph. | ||
| cleared: set[ContainerObjectIdentifier] = set() |
There was a problem hiding this comment.
nit: cleared also makes sense, but I think something like cycle_free_objects / known_cycle_free_objects is a bit more self-documenting.
There was a problem hiding this comment.
Done — went with known_cycle_free_objects.
The set holds the objects already established to be cycle-free rather than every object that happens to be one, so the longer of your two suggestions seemed worth the characters: it states that outright instead of leaving it to be inferred from entries only being added after the subtree returns.
Pushed in d2e15e1. The workflow run is sitting on action_required and needs your approval to go green again.
Unrelated to this PR, but feel free to also check out #116, might be useful with a graph of this size. |
Per review. The set holds the objects already established to be cycle-free rather than every object that happens to be one, and the longer name says so without a reader having to infer it from the fact that entries are added only after the subtree returns.
|
Thank you @Max-Moro |
2 participants
Problem
assert_valid_resolution_pathdetects cycles by walking the dependency graphdepth-first, but it enumerates every distinct root-to-leaf path and never records
that a node's subtree already came back clean. In a graph where many injectables
share dependencies — the normal shape once a few services sit on top of a common
core — each shared subgraph is re-walked once per incoming path, so the visit
count grows with the number of paths through the graph rather than with its
size. Container creation pays this on every startup.
It goes unnoticed easily: for a long-lived web app the container is built once
per deploy. It surfaced here in a CLI, where the container is built once per
invocation, and an IDE addon calls that CLI on every user interaction.
Fix
Thread a set of cleared object ids through the walk and return early for any
dependency already in it, adding to it only once the whole subtree has returned
without raising.
Skipping a cleared dependency is sound: were it part of a cycle, the walk would
have come back to it while it was still on the current path — and that check,
along with the resulting error message, is unchanged. Marking happens after the
recursion, so a node still on the stack is never treated as cleared.
The set is created in
validate_registryand lives for exactly one run. It isdeliberately not module level: two registries describe two different graphs, and
a node cleared in one says nothing about the other.
Numbers
Synthetic graph, 316 injectables, fan-out 3, depth 9:
validate_registryOn the real ~320-type application container that prompted this: 418,891 visits
to 550, cutting 1.40 s off a 1.97 s container build.
Tests
Three added to
test/unit/test_container_creation.py:The last two assert the verbatim error message, so the diagnostic is pinned as
well as the detection.
Because the latter two pass on unmodified
mastertoo, I verified they haveteeth by breaking the fix on purpose:
tests and the pre-existing
test_validates_container_raises_when_cyclical_dependencies;clearedmodule level — fails exactly the cross-container test, andnothing else.
Both mutations surface as
RecursionErrorin_is_dependency_async: a cycle thatslips past validation kills container creation on the very next step. That is why
the skip has to be provably sound rather than merely plausible.
Verification
check-fmt,check-ruff,check-mypy --strictandcheck-docsare clean.make testis green apart from 6 pre-existing failures intest/unit/test_inject_from_container.py, unrelated to this change: those testsmatch
0x[0-9a-f]+against a functionrepr, and CPython on Windows prints theaddress in uppercase hex. Confirmed identical on pristine
master, three runseach side, on both 3.11 and 3.14. They should be green on CI.
assert_valid_resolution_pathgains a sixth parameter and so tripsPLR0913;suppressed with
# noqafollowing the six existing uses of that suppression inwireup/.Unrelated observation
Not touched here, but noticed while reading
validate_registry: a dependency thatis unknown but has a default is removed from
registry.dependencies[factory]only after that factory's own loop finishes. If a different factory is validated
earlier and its walk descends into this one, the walk iterates the not-yet-cleaned
dependency dict and reaches
factories[object_id]with a missing key, raisingKeyError. This is present onmaster; memoisation strictly reduces the set ofvisited nodes, so it can only make it less likely, never more. Happy to open a
separate issue if useful.
The cycle validation this changes was added in #83 for #80.