perf(core-py): share one Tokio runtime across session contexts - #2510
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Walkthrough
ChangesRuntime migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SessionContext
participant shared_runtime
participant TokioRuntime
participant ForkedProcess
SessionContext->>shared_runtime: request runtime for transform_sql
shared_runtime->>TokioRuntime: execute blocking SQL transformation
TokioRuntime-->>SessionContext: return transformed SQL
SessionContext->>ForkedProcess: fork process
ForkedProcess->>shared_runtime: request runtime after fork
shared_runtime->>TokioRuntime: recreate runtime for child PID
TokioRuntime-->>ForkedProcess: return working runtime
ForkedProcess->>SessionContext: transform SQL in child
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/wren-core-py/src/context.rs (1)
130-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDefer fetching the shared runtime.
The
runtimevariable is fetched at the beginning of thenewmethod but remains unused in theNonebranch (lines 132-152) sinceregister_function_by_data_sourcefetches its own runtime reference internally. To slightly reduce initialization overhead, consider deferring theshared_runtime()?fetch to where it is actually needed inside theOkbranch ofAnalyzedWrenMDL::analyze.♻️ Proposed refactor
Remove the early fetch:
) -> PyResult<Self> { - let runtime = shared_runtime()?; - let Some(mdl_base64) = mdl_base64 else {And move it inside the
Okblock around line 219:match AnalyzedWrenMDL::analyze( manifest, Arc::clone(&properties_ref), mdl::context::Mode::Unparse, ) { Ok(analyzed_mdl) => { let analyzed_mdl = Arc::new(analyzed_mdl); + let runtime = shared_runtime()?; let unparser_ctx = py .detach(|| { runtime.block_on(apply_wren_on_ctx(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/wren-core-py/src/context.rs` around lines 130 - 152, Defer the shared runtime lookup in the `new` method: remove the early `shared_runtime()?` assignment before the `mdl_base64` branch, then fetch it only in the `Some`/successful analyzed-MDL path where `runtime` is required. Keep the `None` branch unchanged and preserve existing error propagation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@core/wren-core-py/src/context.rs`:
- Around line 130-152: Defer the shared runtime lookup in the `new` method:
remove the early `shared_runtime()?` assignment before the `mdl_base64` branch,
then fetch it only in the `Some`/successful analyzed-MDL path where `runtime` is
required. Keep the `None` branch unchanged and preserve existing error
propagation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ee2f7b58-a99d-4157-9fc9-f7389dd02b24
📒 Files selected for processing (2)
core/wren-core-py/src/context.rscore/wren-core-py/tests/test_modeling_core.py
Before this change, each PySessionContext created a multi-thread Tokio runtime and spawned roughly one worker per CPU for the lifetime of the session. Eight contexts produced 145 native threads in the benchmark. Share one runtime per process behind a PID-keyed accessor. A forked child rebuilds it lazily; leak the inherited runtime state because dropping it may wait for worker threads that fork did not copy.
A forked child inherits the runtime state but not its worker threads. Verify that contexts created before and after fork both work after the PID-keyed accessor rebuilds the runtime. The child reports result mismatches and exceptions through its exit status. Bound the parent wait so deadlock regressions fail instead of hanging CI.
db627e8 to
8a6ba50
Compare
Skipped. Although the local Keeping the eager lookup also preserves fail-fast runtime creation errors during Python context construction. Deferring it would only avoid initialization for a subset of paths, such as BigQuery, while changing when those errors are reported. |
Problem
Each context previously added about 18 runtime worker threads, producing
an approximately
1 + 18 × contextsnative-thread count on this machine.With the shared runtime, the count stays flat:
Measured on an 18-core macOS machine:
Changes
before and after the fork.
hanging CI.
A forked child inherits the runtime handle but not its worker threads.
The inherited handle is deliberately leaked because dropping it may
wait for workers that were not copied by
fork().The Python API is unchanged. Runtime usage outside
wren-core-pyisnot affected.
Related work
#2485 releases the GIL around Rust execution while retaining the
per-context
call_lock.#2495 fixes a separate catalog-list race exposed by concurrent
transforms. It is now merged and complements this PR: #2495 isolates
catalog state, while this PR bounds Tokio worker growth.
#2504 tracks the remaining Python-side catalog documentation,
integration coverage, and eventual
call_lockremoval. This PR doesnot change catalog semantics or close that issue.
Verification
cargo test— 37 passedcargo test --no-default-features— 37 passedcargo clippy --all-targets --all-features -- -D warningsmaturinSummary by CodeRabbit
Bug Fixes
Tests
fork()on POSIX systems, including deadline-based polling, timeout handling, and exit-status reporting.