test: add direct-call baseline and threaded-contention benchmarks - #129
Conversation
Close the two gaps left open in bagowix#84 after the CodSpeed suite landed: - test_baseline_direct_call and its async twin measure the unwrapped callable, so any call-path result divides into an overhead ratio — the number a prospective user actually wants. - benchmarks/test_contention.py drives one closed breaker from four worker threads to track the work under the shared lock as a trend. The docstring and CONTRIBUTING.md spell out that instruction counting runs threads one at a time: the number is lock-path work, not wall-clock contention.
…-contention # Conflicts: # CHANGELOG.md
bagowix
left a comment
There was a problem hiding this comment.
Thanks for picking this up — this closes out #84 exactly as scoped in the status check, and the execution is careful: the pool is reused across iterations so thread startup stays out of the measurement, the warm-up assert both validates correctness and fills the window to steady state before measuring, and the Valgrind caveat is spelled out right where the number will be read. I ran the full gate locally on the branch (ruff, mypy, pyright, pyrefly, full suite at 100% coverage, pytest benchmarks --codspeed -n 0) — all green, and the workflow picks up the new module with no changes, as advertised.
One non-blocking nit, take it or leave it: the async baseline is slightly asymmetric with test_call_async. The breaker path runs a wrapper coroutine (guarded) that awaits breaker.call(...), while the baseline runs the bare _async_work coroutine with no wrapper — so the overhead ratio includes one extra coroutine frame that real usage doesn't pay (in real code the user's own coroutine exists in both cases: await fn(...) vs await breaker.call(fn, ...)). Wrapping the baseline the same way (async def bare(): return await _async_work(1, 2)) would make the ratio exactly apples-to-apples. For a trend metric it's noise either way, so happy to merge as is.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (7)
WalkthroughThe benchmark suite adds synchronous and asynchronous direct-call baselines and a four-worker threaded contention benchmark. Documentation explains baseline comparisons and serialized instruction-count measurements. ChangesBenchmark coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Benchmark
participant ThreadPoolExecutor
participant CircuitBreaker
participant Payload
Benchmark->>ThreadPoolExecutor: submit concurrent call batches
ThreadPoolExecutor->>CircuitBreaker: execute protected calls
CircuitBreaker->>Payload: run lightweight payload
CircuitBreaker-->>ThreadPoolExecutor: return payload result
ThreadPoolExecutor-->>Benchmark: return aggregate results
🚥 Pre-merge checks | ✅ 9✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
The breaker path drives a wrapper coroutine that awaits breaker.call, while the baseline ran the bare coroutine — so the overhead ratio charged the breaker for one coroutine frame real usage pays on both sides. Wrapping the baseline the same way makes the ratio isolate the breaker itself.
## Summary Add a `.coderabbit.yaml` so the automated PR review enforces this project's rules instead of generic Python advice. - **Path instructions (11 globs).** Encode the parts of `AGENTS.md` that are invisible in a diff: stdlib-only core outside `interlock/integrations/`, time only through the injected `Clock`, the lock in `_engine.py` never held across the protected call, `X | None` over `Optional[X]`, keyword arguments at 3+ parameters, the `test__unit__state__expected` naming convention, and SHA-pinned actions under `.github/workflows/`. The sanctioned swallow in `_notify.py` is called out explicitly so the bot stops trying to "fix" it. - **Pre-merge checks.** Five custom checks mirror the definition of done: zero-dependency core (`error`), `[Unreleased]` changelog entry, docs plus a regenerated `docs/llms-full.txt`, tests alongside behaviour changes, and public API surface (requires the `breaking-change` label plus a migration note, matching the `griffe check` gate). Title check enforces Conventional Commits, since PRs are squash-merged. - **Noise control.** `pylint` and `markdownlint` are off — ruff and pymarkdown already gate those files, and a second unconfigured rule set only produces contradictory nits. Docstring coverage is `off`: public API carries docstrings, private helpers deliberately do not. - **Safety.** `auto_apply_labels` stays `false` and `breaking-change` is absent from the labeling instructions, because that label switches off the griffe API-compatibility gate — not something a bot should do unattended. - **Knowledge base.** `filePatterns` points at `AGENTS.md` and `CONTRIBUTING.md`; the defaults look for `CLAUDE.md` files, which are gitignored here and would never be visible. `docs/llms-full.txt` is deliberately *not* in `path_filters`: those patterns also drive sparse-checkout, and excluding the file would blind the check that verifies the mirror was regenerated. A path instruction tells the reviewer to confirm its regeneration without reviewing its contents. `reviews.auto_review.base_branches` names `main` explicitly. On installation the bot skipped PR #129 with "auto reviews are disabled on base/target branches other than the default branch", although that PR targets `main` and the base was never changed. The explicit entry removes the dependency on how CodeRabbit resolves the default branch. Validated against `https://coderabbit.ai/integrations/schema.v2.json` — no errors. Note that CodeRabbit reads the file from the branch under review, so this PR is its own smoke test: the Run configuration block should report `Configuration used: .coderabbit.yaml` instead of `defaults`. ## Checklist - [x] Tests added or updated (suite stays at 100% coverage) — n/a, no library code changed; the suite is untouched - [x] `uv run ruff format --check` and `uv run ruff check` pass - [x] `uv run mypy`, `uv run pyright` and `uv run pyrefly check` pass - [x] Docs updated (`docs/`) for user-facing changes — n/a, tooling only - [x] `CHANGELOG.md` `[Unreleased]` updated — n/a, no user-visible change - [x] Commits follow Conventional Commits ## Related issues <!-- none --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ### Added - Added CodeRabbit review gates for dependencies, concurrency, typing, tests, documentation, CI security, changelog updates, and public API changes. - Added Conventional Commit title validation. - Added knowledge-base sources for `AGENTS.md` and `CONTRIBUTING.md`. ### Changed - Added review rules for state-machine behavior, engine lock scope, public API changes, and supported Python versions. - Set `main` as the review base branch. - Disabled `pylint`, `markdownlint`, docstring coverage, and automatic label application. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…ontention # Conflicts: # CHANGELOG.md
Merging this PR will not alter performance
Performance Changes
Comparing |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Summary
Closes the two gaps left open in #84 after the CodSpeed suite landed (per the status check in the issue comments):
test_baseline_direct_callmeasures the unwrapped callable, so any call-path result divides into an overhead ratio — the number a prospective user actually wants. Its async twin runs the bare coroutine on the same pre-builtasyncio.Runnerthe async benchmarks use, so the loop-dispatch cost the async paths pay is present in their baseline too and the ratio stays honest.benchmarks/test_contention.pydrives one closed breaker from four worker threads (25 calls each, pool reused across iterations so thread startup is excluded), tracking the work under the shared lock as a trend. The module docstring andCONTRIBUTING.mdcarry the caveat: CodSpeed's simulation mode counts instructions under Valgrind, which runs threads one at a time — the number is lock-path work, not wall-clock contention. Correctness under real contention stays withtests/test_concurrency.py/ Run the test suite on free-threaded CPython (3.14t) #102.No runtime code is touched; the CodSpeed workflow needs no change (it collects
benchmarks/wholesale).docs/is intentionally untouched — thecorrectness.mdCodSpeed paragraph stays accurate as written; the reading guidance is contributor-facing and lives inCONTRIBUTING.md.Checklist
uv run ruff format --checkanduv run ruff checkpassuv run mypy,uv run pyrightanduv run pyrefly checkpassdocs/) for user-facing changesCHANGELOG.md[Unreleased]updatedRelated issues
Closes #84
Added
test_baseline_direct_callbenchmarks in the benchmark suite.CircuitBreaker.Changed