Repository navigation
Keep monitoring dispatch and event metadata interpreter-local - #9439
youknowdot wants to merge 8 commits into
Conversation
Serialize syslog masks and mmap cursor operations, preserve distinct mmap sequence bytes, and migrate ten unchanged CPython 3.15 concurrency suites after their native prerequisites. Assisted-by: OpenAI Codex: model version unavailable
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] test: cpython/Lib/test/test_free_threading dependencies: dependent tests: (no tests depend on free_threading) Legend:
|
Assisted-by: OpenAI Codex: model version unavailable
Merging this PR will degrade performance by 18.7%1 warning
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
Use immutable source metadata and native code identity for virtual monitoring layers; preserve callback continuations and ordinary branch event sites. Assisted-by: OpenAI Codex: model version unavailable
Assisted-by: OpenAI Codex: model version unavailable
|
hmm, let's check performance before merge @codspeedbot explain performance drop |
Why performance droppedThis is a real regression, not noise. All of it comes from f8a7823 ("Keep monitoring dispatch in interpreter-local state"):
What the flamegraph shows (
|
| c672599 | f8a7823 | |
|---|---|---|
Interpreter loop self time (run_until → run_until_inner) |
366.6 ms | 534.0 ms |
monitoring::dispatch_operation self time |
— | 175.8 ms |
mem::replace<Option<u32>> (inlined) |
— | 39.3 ms |
| Total | 1.3 s | 1.7 s |
The float, list and call code costs the same in both runs (e.g. default_dealloc<PyFloat> 67.9 → 67.2 ms, PyFloat::into_ref 31.7 → 31.8 ms). The added time is all overhead in the dispatch loop, even though no monitoring tool is active.
Where it comes from in the code
-
A function call before every instruction (
crates/vm/src/frame.rs:3909):let mut virtual_dispatch = if PREPARE_GENERATOR { None } else { monitoring::dispatch_operation(vm, self.code, idx) };
dispatch_operation(sys/monitoring.rs:718) is not inlined. Even its fast path (vm.state.monitoring_events.load() == 0) costs a call, a pointer chase throughvm.stateand an atomic load per instruction. It also returns anOption<(MonitoringInstruction, Instruction)>that the loop then branches on. Onmain, the loop goes straight toread_unit_from. This adds extra live values in the hottest loop, which also explains why the loop's own self time grew by ~45%. -
Extra writes per instruction in
execute_instruction(frame.rs:4665-4666):let index = self.lasti().saturating_sub(1); self.monitoring_previous = self.monitoring_current.replace(Some(index));
This re-loads
lasti, even though the loop already holdsidx, and writes twoOption<u32>frame fields on every instruction, monitored or not. -
Smaller additions: the
virtual_line/legacy_line_processedchecks and theDispatchMode/cachestuple on the decode path. Each one is cheap, but they all sit in the loop.
Ways to recover it
- Make the disabled case close to free. Split
dispatch_operationinto an#[inline(always)]check onmonitoring_events == 0and a#[cold] #[inline(never)]slow path. Better still, follow CPython: check instrumentation only at RESUME, backward jumps or the eval breaker, and keep a frame-local "monitored" flag. The plain path would then matchmain. - Track
monitoring_previous/monitoring_currentonly in monitored mode, and take the index fromidxinstead of re-loadinglasti. If the generator-close LINE fix (f1c31d7) needs the previous position in the normal path too, it could be saved only when a frame suspends instead of on every instruction. - Re-run the benchmarks after either change. A plain path that matches
mainshould bring these benchmarks back to their baseline.
I haven't changed any code on the branch. Comment @codspeedbot try making the monitoring fast path zero-cost when no tool is active and I'll prototype the fix and benchmark it.
Summary
- Warming up the performance wizard...
- Isolate the commit causing the regression — c672599 (dict/CSV) is flat vs main; all 24 regressions appear at f8a7823 (monitoring dispatch).
- Compare flamegraphs to find hot spots — New per-instruction work in the dispatch loop: out-of-line monitoring::dispatch_operation call plus monitoring_previous/current bookkeeping.
- Report findings — Report findings
- Handling comment — Comment handling finished successfully
Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.
Preserve original RustPython#9439 ancestry and the reviewed float-power integration while preparing its functionality split. Assisted-by: OpenAI Codex: model version unavailable
Move dict, CSV, mmap, syslog and library-only modules into their coherent independent units while preserving complete monitoring source and its whole canonical test. Keep the provenance overlap explicit until its independent PR lands. Assisted-by: OpenAI Codex: model version unavailable
Extract the six canonical library-only modules from #9439. Regenerated with scripts/update_lib from CPython d220409a13b77de7a6c30abe16ea90db01528edc. Assisted-by: OpenAI Codex: model version unavailable Co-authored-by: Codex <[email protected]>
Extract the syslog lock and whole canonical free-threading test from #9439. Regenerate the test with scripts/update_lib from CPython d220409a13b77de7a6c30abe16ea90db01528edc. Assisted-by: OpenAI Codex: model version unavailable Co-authored-by: Codex <[email protected]>
Extract mmap cursor and sequence behavior plus its whole canonical free-threading test from #9439. Regenerate the test with scripts/update_lib from CPython d220409a13b77de7a6c30abe16ea90db01528edc. Assisted-by: OpenAI Codex: model version unavailable Co-authored-by: Codex <[email protected]>
Extract CSV reader synchronization with its PyOperation facade and float-error helper from #9439. Include the whole canonical free-threading CSV test regenerated by scripts/update_lib from CPython d220409a13b77de7a6c30abe16ea90db01528edc. Assisted-by: OpenAI Codex: model version unavailable Co-authored-by: Codex <[email protected]>
Extract dictionary insertion and setdefault re-probing from #9439 with its five structural fixtures and whole canonical free-threading test. Regenerate the Lib file with scripts/update_lib from CPython d220409a13b77de7a6c30abe16ea90db01528edc. Assisted-by: OpenAI Codex: model version unavailable Co-authored-by: Codex <[email protected]>
Assisted-by: OpenAI Codex: model version unavailable
Use byte-width instruction reads and bounded base-family traversal. Remove the external-bytecode trust and frozen import policy changes from this monitoring patch. Assisted-by: OpenAI Codex: model version unavailable
Summary
Keep monitoring dispatch and layer state interpreter-local without rewriting shared bytecode into monitoring wrappers.
The metadata substrate no longer depends on #9450: use byte-width atomic instruction reads and a bounded base-family walk that skips inline caches, rather than a permanent construction-provenance copy. Remove the new external-bytecode rejection/trust framework and frozen/import fallibility. Preserve existing constructor,
code.replace, and marshal admission behavior, ordinary owning-code normalization, and literalGET_ITERconstruction order.This follows CPython's base-operation/deoptimization conventions, not a claim to port its critical-section or thread-local bytecode machinery. Existing generic internal Clone/cache safety is outside this correction. The current-PC legacy trace path avoids an unnecessary prefix scan; arbitrary-offset metadata lookups remain bounded linear scans.
Verification
The final correction is byte-for-byte identical across all 12 changed paths to the locally validated source on b85582d. Main 698c196 was merged normally afterward; its seven SQLite/overlapped/regex-related paths do not overlap this correction. No fresh runtime pass is claimed for that merged base; current-head CI remains pending.
--lib --tests) and VM (--lib, compiler/threading/host_env/stdio/doc/importlib) check and strict Clippy passed on the identical monitoring correction before the disjoint current-main dictionary update.-lffilinker name was resolved using an explicit local link to the existing system libffi; no repository or system-library modification.co_code, enable both monitoring events, first-read normalized bytes,replace/marshal round trips, execute ordinary copies. Comparisons are within each engine, not physical bytecode equivalence across engines.Full workspace/C-API suites, broad monitoring/free-threading suites, concurrency/GC/stress probes, and performance validation were not run. The reviewer's performance-before-merge request remains open; this correction does not claim to resolve overall dispatch overhead. No merge readiness claim.
AI assistance: OpenAI Codex assisted source audit, implementation, scoped validation, and this description; model version unavailable.