perf(container): unlock container reopen, drop the context-resolve cast - #404
Merged
Merged
Conversation
`_prepare()` acquired the container lock and re-checked `closed` so that N
threads racing a closed container produced exactly one warning and one reopen.
The lock bought nothing the callers did not already have: every caller guards
with `if closed`, and the reopen is idempotent -- every racing thread writes the
same `closed = False`. `open()` took the same lock for the same single store.
Both are now unlocked. The cost is a weaker warning contract: concurrent reuse
of one closed container warns *at least once* rather than exactly once. Under
the default warning filters Python's per-location registry collapses the
duplicates anyway; `simplefilter("always")` reveals them.
Also drops `typing.cast` from `ContextProvider.resolve`, a Python-level call on
the context resolve path, measured at ~19ns. `fetch_context_value` and
`find_context` now annotate their true return (`T | UnsetType`) instead of
widening to `object`; `is UNSET` does not narrow in ty, so the return carries a
scoped `ty: ignore` rather than paying ~10ns for `isinstance`.
Verified: 452 tests, 100% coverage; free-threading suite 200x under GIL and
100x under 3.14t.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Benchmark
Details
| Benchmark suite | Current: 51dc34c | Previous: c2f11a6 | Ratio |
|---|---|---|---|
benchmarks/test_guard_by_type.py::test_g16_resolve_by_type |
2139286.9908109037 iter/sec (stddev: 2.5839608361232744e-7) |
2614526.809296543 iter/sec (stddev: 4.949782770563345e-8) |
1.22 |
benchmarks/test_guard_by_type.py::test_g17_resolve_by_type_large_registry |
2601614.3791889837 iter/sec (stddev: 5.698210072768198e-8) |
2575610.685619507 iter/sec (stddev: 6.530521439509241e-8) |
0.99 |
benchmarks/test_guard_cold.py::test_g8_cold_first_resolve |
26879.354500741774 iter/sec (stddev: 0.000015941557849166365) |
26327.708985457688 iter/sec (stddev: 0.00000987833118866444) |
0.98 |
benchmarks/test_guard_concurrency.py::test_g14_concurrent_cached_hit[1] |
390.68638733691034 iter/sec (stddev: 0.00007799404027731494) |
383.6833846733762 iter/sec (stddev: 0.00008740193339905497) |
0.98 |
benchmarks/test_guard_concurrency.py::test_g14_concurrent_cached_hit[2] |
382.0132845120135 iter/sec (stddev: 0.0000460026221063088) |
366.10019224224067 iter/sec (stddev: 0.0000340410601438076) |
0.96 |
benchmarks/test_guard_concurrency.py::test_g14_concurrent_cached_hit[4] |
307.4204522954904 iter/sec (stddev: 0.0003281527139844629) |
337.2946037697783 iter/sec (stddev: 0.00003193231890467563) |
1.10 |
benchmarks/test_guard_concurrency.py::test_g15_concurrent_first_resolve[1] |
2445.116158607276 iter/sec (stddev: 0.000028738656750639723) |
2363.6839345663348 iter/sec (stddev: 0.0000640542285548261) |
0.97 |
benchmarks/test_guard_concurrency.py::test_g15_concurrent_first_resolve[2] |
1695.0300039380695 iter/sec (stddev: 0.00034545695487124355) |
1732.5672190592084 iter/sec (stddev: 0.00023596124331628116) |
1.02 |
benchmarks/test_guard_concurrency.py::test_g15_concurrent_first_resolve[4] |
1194.2866520847613 iter/sec (stddev: 0.000036480129430528174) |
1215.4055448241975 iter/sec (stddev: 0.00003373681764753561) |
1.02 |
benchmarks/test_guard_lifecycle.py::test_g6_build_child_container |
692318.0822596957 iter/sec (stddev: 4.646928336942312e-7) |
684628.4187950536 iter/sec (stddev: 4.515673776018582e-7) |
0.99 |
benchmarks/test_guard_lifecycle.py::test_g6b_build_child_container_auto_scope |
650192.7002192794 iter/sec (stddev: 4.262791642594767e-7) |
645745.8621762114 iter/sec (stddev: 4.1164296068011774e-7) |
0.99 |
benchmarks/test_guard_lifecycle.py::test_g7_request_lifecycle_batch |
2388.983818684372 iter/sec (stddev: 0.00000987912141354149) |
2201.914920036075 iter/sec (stddev: 0.000011834323722317028) |
0.92 |
benchmarks/test_guard_lifecycle.py::test_g7c_event_loop_floor_control |
62763.920859509955 iter/sec (stddev: 0.000002051739107988561) |
61404.5143191709 iter/sec (stddev: 0.0000023326005370671473) |
0.98 |
benchmarks/test_guard_lifecycle.py::test_g13_teardown_at_scale |
46680.15229807799 iter/sec (stddev: 0.0000021357643518541776) |
45968.76841084101 iter/sec (stddev: 0.0000019507899000292644) |
0.98 |
benchmarks/test_guard_resolve.py::test_g1_transient_resolve |
1339207.503042861 iter/sec (stddev: 3.2823483155471444e-7) |
1322213.200265449 iter/sec (stddev: 3.4573104835434364e-7) |
0.99 |
benchmarks/test_guard_resolve.py::test_g2_cached_resolve |
3222188.2725913683 iter/sec (stddev: 4.7611673526330975e-8) |
3167355.480250035 iter/sec (stddev: 6.348456212488457e-8) |
0.98 |
benchmarks/test_guard_resolve.py::test_g3_deep_chain |
522207.92555050313 iter/sec (stddev: 6.96151638671523e-7) |
519186.5375634182 iter/sec (stddev: 5.001374482496076e-7) |
0.99 |
benchmarks/test_guard_resolve.py::test_g4_wide_resolve |
327706.8930434782 iter/sec (stddev: 5.399113839851165e-7) |
215724.83913037705 iter/sec (stddev: 0.000001494757761145363) |
0.66 |
benchmarks/test_guard_resolve.py::test_g5_cross_scope |
1121455.833219079 iter/sec (stddev: 5.07319760432924e-7) |
1125680.0884182719 iter/sec (stddev: 5.088598076879609e-7) |
1.00 |
benchmarks/test_guard_resolve.py::test_g9_context_resolve |
581397.2034331578 iter/sec (stddev: 0.0000011054125578709495) |
573907.7838913013 iter/sec (stddev: 5.999734071221531e-7) |
0.99 |
benchmarks/test_guard_resolve.py::test_g12_override_active_resolve |
400777.96295500215 iter/sec (stddev: 3.881678630442111e-7) |
383365.44356512895 iter/sec (stddev: 4.0062760290095966e-7) |
0.96 |
benchmarks/test_guard_validate.py::test_g10_validate_deep_chain |
29325.155160086328 iter/sec (stddev: 0.0000033565540892635954) |
29254.783252128505 iter/sec (stddev: 0.000003434039494489166) |
1.00 |
benchmarks/test_guard_validate.py::test_g11_validate_wide |
17675.188365953294 iter/sec (stddev: 0.000004864069340176364) |
18121.03714812697 iter/sec (stddev: 0.00000402532787348672) |
1.03 |
This comment was automatically generated by workflow using github-action-benchmark.
5 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
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
Two locks on the resolve path that bought nothing.
_prepare()— the self-heal the resolve path calls whenever it findsclosed = True— acquired the container'sRLockand re-checkedclosedunder it, so that N threads racing one closed container produced exactly oneContainerClosedWarningand one reopen. But every caller already guards withif closed, and the write itself is idempotent: each racing thread stores the sameclosed = False. The lock was serialising threads to deduplicate a warning, on a path whose entire job is to recover from misuse.open()took the same lock for the same single store.Separately,
ContextProvider.resolveended withtyping.cast(types.T_co, value).typing.castis a real Python function call, so it cost a frame on every context resolve (the G9 path) purely to satisfy the type checker.Design
Both reopens become plain unlocked stores.
_prepare()warns and clearsclosedunconditionally; the caller'sif closedis the only guard.The trade-off is a genuinely weaker contract, and it is stated rather than hidden: concurrent reuse of one closed container warns at least once, not exactly once. Threads that race all warn. Under default warning filters, Python's own per-location
__warningregistry__collapses the duplicates, so this is invisible unless a caller setssimplefilter("always"). What is preserved is what actually matters: all racing threads still converge on one open container and share one singleton, via the cache lock, which is untouched.For the cast: rather than delete it and widen the annotation, the annotations are corrected to what these methods actually return.
fetch_context_valueandContextRegistry.find_contextsaidT | object, which is true but useless — they returnTorUNSET, never arbitraryobject. They now sayT | UnsetType.is UNSETdoes not narrow inty(UNSETis aFinalinstance, not a tracked singleton), so a choice remained. Measured, per call, including ~20ns of harness overhead in each:is UNSET+typing.cast(before)isinstance(value, UnsetType)is UNSET+ty: ignore(chosen)Chose the fastest and paid for it with one scoped, commented
ty: ignoreon a single return.Non-goals
architecture/concurrency.mdalready documents. This PR only changes the reuse-after-close path, which is the race that is handled.typing.castinContextRegistry.find_context. Same micro-cost, same path, but out of scope here.Verification
just test-ci— 452 passed, 100% line coverage (the gate).just lint-ci— clean, includingruff,ty, planning validation, and link checking.tests/test_free_threading.pyrun 200x under the GIL build and 100x under free-threaded 3.14t (cpython-3.14.6+freethreaded), all passing — this is where an unlocked reopen would surface if it were unsafe.just bench, whose guard medians are quantised to a 41ns tick and cannot resolve a move this size — the methodbenchmarks/README.mdprescribes for small changes.One honesty note for review:
test_concurrent_reuse_after_close_warns_and_reopensnow asserts>= 1warnings, so it no longer distinguishes this implementation from the locked one. That is inherent to relaxing the contract — "at least once" is not falsifiable by counting. Its load-bearing assertions are the ones that survived: all threads receive the same singleton, and the container ends open.Before merging
architecture/concurrency.mdrewritten for the at-least-once contract;architecture/containers.md"Open and reopen" corrected — it still described the under-lock re-check.just lint-ciandjust test-cipass.