perf(container): inline the resolve_provider body into resolve - #419
Merged
Conversation
Container.resolve called find_provider and then tail-called resolve_provider, paying a second Python frame on the by-type path -- the one every @Inject marker and framework integration takes. It now carries its own copy of that body. Measured -19% (~38 ns) per by-type resolve on g16_by_type, with the by-reference control flat. The duplicated 8-line body is the standing cost and is permanent: both copies must be edited together. Licensed by planning/decisions/2026-08-03-resolve-provider-not-a-seam.md, which records that resolve_provider stopped being an interception seam when the compiled resolvers landed in 2.29.0 -- an override already sees only top-level calls, demonstrated at 1 call for a 4-node chain. The 3.10 coverage gate needed more than the deferred item predicted. A by-reference cycle test does reach resolve_provider's RecursionError handler and fails without it, but coverage cannot record that line when tracing the whole modern_di package below 3.12: the RecursionError tears the trace function down first. It records fine when tracing container.py alone, which is how the earlier check missed this. The line therefore carries a pragma with the evidence, and the test stays as the real guard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The review refuted the pragma and its justification. The comment claimed resolve_provider's handler is "reached only by a by-reference cycle" -- false: a by-type cycle re-enters resolve_provider through the back-edge thunk, hitting it six times. What actually changed is that the two tests reaching that line WITHOUT a stack overflow (SelfRec raises RecursionError directly, tracer intact) go by type, so they now land on resolve's copy. The fix is a by-reference twin of that overflow-free shape, not a pragma: test_by_reference_recursionerror_passes_through. 3.10 and 3.14 both back to 100.00% with no suppression on a production line. The cycle test stays, with a corrected comment: it contributes no coverage but is the sole behavioural guard on resolve_provider's own conversion -- replacing that conversion with a bare re-raise kills only that test. Also corrects four architecture pages the review found still naming resolve_provider as the sole compile-and-dispatch path: resolution.md's entry point list, performance.md's inlined-lookup counts (four across three call sites -> six across four) plus a note recording the duplication where a reviewer would look for it, concurrency.md's reopen paragraph, and containers.md's self-heal line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Benchmark
Details
| Benchmark suite | Current: 7c16a75 | Previous: df72b27 | Ratio |
|---|---|---|---|
benchmarks/test_guard_by_type.py::test_g16_resolve_by_type |
3120093.110328125 iter/sec (stddev: 1.6044443634779525e-7) |
2069083.004621692 iter/sec (stddev: 2.507917057686009e-7) |
0.66 |
benchmarks/test_guard_by_type.py::test_g17_resolve_by_type_large_registry |
4029025.566120594 iter/sec (stddev: 3.0745992653975385e-8) |
2615296.8915319657 iter/sec (stddev: 5.651557610136808e-8) |
0.65 |
benchmarks/test_guard_cold.py::test_g8_cold_first_resolve |
33638.779315974614 iter/sec (stddev: 0.000015307240780211678) |
22790.17870881894 iter/sec (stddev: 0.00002041723075498584) |
0.68 |
benchmarks/test_guard_concurrency.py::test_g14_concurrent_cached_hit[1] |
534.2113749223787 iter/sec (stddev: 0.000059018704694738065) |
424.87527220904894 iter/sec (stddev: 0.0004121088158995983) |
0.80 |
benchmarks/test_guard_concurrency.py::test_g14_concurrent_cached_hit[2] |
526.5030722417341 iter/sec (stddev: 0.00002323346904833162) |
416.3562771945866 iter/sec (stddev: 0.000036508398217920814) |
0.79 |
benchmarks/test_guard_concurrency.py::test_g14_concurrent_cached_hit[4] |
472.9583171271513 iter/sec (stddev: 0.000034680303981284446) |
376.79541359036506 iter/sec (stddev: 0.00011842281765237235) |
0.80 |
benchmarks/test_guard_concurrency.py::test_g15_concurrent_first_resolve[1] |
3188.307837485942 iter/sec (stddev: 0.000021360373113886245) |
2297.838029302786 iter/sec (stddev: 0.00002304186737928606) |
0.72 |
benchmarks/test_guard_concurrency.py::test_g15_concurrent_first_resolve[2] |
2252.0140324616564 iter/sec (stddev: 0.00028848924734000813) |
1660.5461013051608 iter/sec (stddev: 0.000313793147249187) |
0.74 |
benchmarks/test_guard_concurrency.py::test_g15_concurrent_first_resolve[4] |
1580.213764207307 iter/sec (stddev: 0.000026706225311668934) |
1189.1402008980458 iter/sec (stddev: 0.000037042506386042706) |
0.75 |
benchmarks/test_guard_lifecycle.py::test_g6_build_child_container |
852102.3738079976 iter/sec (stddev: 3.354221736510672e-7) |
686119.0697788467 iter/sec (stddev: 4.6940566591711673e-7) |
0.81 |
benchmarks/test_guard_lifecycle.py::test_g6b_build_child_container_auto_scope |
822367.1961764119 iter/sec (stddev: 2.778770952762505e-7) |
641187.6411146931 iter/sec (stddev: 0.000001714956882750074) |
0.78 |
benchmarks/test_guard_lifecycle.py::test_g7_request_lifecycle_batch |
3004.901791775335 iter/sec (stddev: 0.0000068912586202921525) |
2345.644128281843 iter/sec (stddev: 0.00001831805508907972) |
0.78 |
benchmarks/test_guard_lifecycle.py::test_g7c_event_loop_floor_control |
79082.74428830792 iter/sec (stddev: 9.707509479853904e-7) |
61722.88012886701 iter/sec (stddev: 0.000001992769465067329) |
0.78 |
benchmarks/test_guard_lifecycle.py::test_g13_teardown_at_scale |
57081.18321741825 iter/sec (stddev: 0.0000011440285991187668) |
45899.425335696775 iter/sec (stddev: 0.000001939256591546186) |
0.80 |
benchmarks/test_guard_resolve.py::test_g1_transient_resolve |
1733142.315318067 iter/sec (stddev: 2.7648226171258356e-7) |
1320109.5945375036 iter/sec (stddev: 5.937493646314103e-7) |
0.76 |
benchmarks/test_guard_resolve.py::test_g2_cached_resolve |
4365216.13625033 iter/sec (stddev: 2.676294413735727e-8) |
3275300.035003006 iter/sec (stddev: 5.597398639493469e-8) |
0.75 |
benchmarks/test_guard_resolve.py::test_g3_deep_chain |
672294.0041754248 iter/sec (stddev: 4.008092389474844e-7) |
504536.652359001 iter/sec (stddev: 8.772474778880065e-7) |
0.75 |
benchmarks/test_guard_resolve.py::test_g4_wide_resolve |
414235.0980633594 iter/sec (stddev: 6.263032775870878e-7) |
323936.97446466243 iter/sec (stddev: 6.842273223175946e-7) |
0.78 |
benchmarks/test_guard_resolve.py::test_g5_cross_scope |
1540236.5353379105 iter/sec (stddev: 3.218297530687242e-7) |
1139987.0502917084 iter/sec (stddev: 4.2653052728991444e-7) |
0.74 |
benchmarks/test_guard_resolve.py::test_g9_context_resolve |
946858.3037378176 iter/sec (stddev: 3.1215870975981575e-7) |
727252.7108060394 iter/sec (stddev: 4.98267196602221e-7) |
0.77 |
benchmarks/test_guard_resolve.py::test_g12_override_active_resolve |
528109.6544380087 iter/sec (stddev: 2.3568326233853274e-7) |
384841.23022941116 iter/sec (stddev: 4.428683652699535e-7) |
0.73 |
benchmarks/test_guard_validate.py::test_g10_validate_deep_chain |
39517.836553063506 iter/sec (stddev: 0.0000016996404861814087) |
27778.772669309943 iter/sec (stddev: 0.0000046446570592294925) |
0.70 |
benchmarks/test_guard_validate.py::test_g11_validate_wide |
23402.35113090448 iter/sec (stddev: 0.000002125197008400135) |
16775.33176726544 iter/sec (stddev: 0.0000057784039244808935) |
0.72 |
This comment was automatically generated by workflow using github-action-benchmark.
4 tasks
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
1 participant
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.
Why
Container.resolve(dependency_type)looked the provider up and then tail-calledself.resolve_provider(provider), paying a second Python frame on the by-type path — the one every@injectmarker and framework integration takes.Retires
planning/deferred/2026-08-01-resolve-by-type-inline.md.Design
resolvenow carries its own copy ofresolve_provider's body: closed check, memo hit,resolver_forfallback, resolver call,RecursionErrorconversion.The duplicated ~8-line body is the standing cost, and it is permanent — both copies must be edited together. That is the trade this PR asks you to have made, and it is recorded in the code, in
architecture/performance.md, and in the decision doc.What licensed it:
resolve_providerstopped being an interception seam when the compiled resolvers landed in 2.29.0. Resolvers call each other directly; its only callers areresolve,resolve_dependency, and the cycle back-edge thunk. AContainersubclass overriding it records 1 call while resolving a 4-node chain onmain— it has never seen the graph. Full reasoning, including why this is deliberately narrower than2026-08-01-scope-map-inline-declined.md(which stands), is inplanning/decisions/2026-08-03-resolve-provider-not-a-seam.md. The reviewer verified that central claim independently: 1 call on main, 0 on this branch, 1 by reference on both.Non-goals
find_containeris untouched and remains a blessed extension point. This rules on one method, not onContainersubclassing generally.resolve_provider, not that subclassing is forbidden.Verification
just test-ci: 477 passed, 100% line coverage on both 3.14 and 3.10 (3.10 run under a local interpreter, twice, since it is the version that gates this change).just lint-ci: clean.Measured with the repo's A/B/A harness (
ab_run.sh main HEAD g16_by_type), CPython 3.14, three runs: -18.57%, -20.10%, -22.21% (~40 ns), by-reference controlg2_cachedflat at +1.13%. Naming the scenario and version because the size is shape-dependent — the reviewer measured a differently-built by-type case at -10.6% on 3.14 and -37.8% on 3.10.One thing I got wrong, and the review caught
I previously reported the 3.10 coverage blocker as "verified fixable in ~10 lines of test", and PR #416 recorded that in the deferred item (now deleted by this PR). That verification was flawed — I checked with
--cov=modern_di.container, and the line records fine under narrow tracing. Under the real gate (--cov=modern_di) it does not.My first fix was a
# pragma: no coveron the production line, with a comment claiming the handler is "reached only by a by-reference cycle". The review refuted both:resolve_providerthrough the back-edge thunk and hits that line six times.SelfRec.__init__raisesRecursionErrordirectly, so the tracer survives) go by type, and now land onresolve's copy.test_by_reference_recursionerror_passes_throughrestores genuine 100% on 3.10.No
pragma: no coveron any production line in this PR. The cycle test stays with a corrected comment: it contributes no coverage, but it is the only guard onresolve_provider's own conversion — replacing that conversion with a bare re-raise kills that test and nothing else.Known follow-up, not in this PR
docs/introduction/performance.mdandbenchmarks/README.mdquote the by-type surcharge (60/54/65 ns; "roughly 40 ns on top ofresolve_provider"). This change invalidates those figures. They come from the generated comparative table, which is republished at release time — hand-editing generated numbers here would be wrong, so this is flagged rather than done.Before merging
architecture/resolution.md,performance.md(counts corrected four/three -> six/four, plus a note recording the duplication where a reviewer would look for it),containers.md,concurrency.md,validation.md.planning/decisions/2026-08-03-resolve-provider-not-a-seam.md.just lint-ciandjust test-cipass, on 3.14 and 3.10.