Repository navigation
Keep monitoring dispatch and event metadata interpreter-local - #9439
youknowdot wants to merge 10 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. CodeRabbit Verify
|
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] test: cpython/Lib/test/test_free_threading (TODO: 1) 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
Preserve original RustPython#9439 ancestry and the reviewed float-power integration while preparing its functionality split. 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 <codex@openai.com>
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 <codex@openai.com>
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 <codex@openai.com>
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 <codex@openai.com>
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 <codex@openai.com>
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
Extract the independent code-source, clone, co_code and fallible decoding foundation from #9439. Interpreter-local monitoring dispatch remains in the dependent original PR. Assisted-by: OpenAI Codex: model version unavailable Co-authored-by: Codex <codex@openai.com>
Keep arbitrary-offset source_operation unchanged. Use a fixed-size same-owner walker for sorted offsets in the three generator paths, preserving guards, duplicate-zero handling, instruction extent checks and callback bookkeeping. The batched lookup is not an atomic snapshot of concurrent monitoring changes. Validation of the pre-format candidate: - Real compiler-core iterator scalar audit: 2 tests passed. - VM cargo check and strict Clippy passed offline with one job and VM opt3. - Six canonical generator/monitoring methods passed on pinned CPython 3.15, original b439d42 and candidate: 18 ordinary successes, no skips/xfails. - A fixed public gi_yieldfrom/full-consumption diagnostic (prefix 8/32/64, four balanced pairs, 20 warmups and 2000 calls per sample) had median candidate/baseline elapsed ratios 0.5046/0.4792/0.4769. All 24 samples and checksums were retained. These are local dev-profile builds with VM opt3, not release/LTO/CodSpeed results or whole-PR performance clearance. O(prefix) traversal remains; these specific cases roughly halve elapsed. After testing, rustfmt only expanded one array literal and added its optional trailing comma; exact before/after replacement was independently checked. The formatted source is not byte-identical to the tested artifact source. Broader workspace/CAPI and release suites were not run locally; hosted suites remain required, including existing close/throw and frame-jump coverage. Implementation, audit drivers and validation summary were AI-assisted. Assisted-by: OpenAI Codex: model version unavailable
a2c7122 to
fac8d18
Compare
Preserve frame inspection claims while using normalized monitoring operations for suspended delegates. 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.