perf: eliminate macOS discovery timeout tail (Fixes #504) - #506
perf: eliminate macOS discovery timeout tail (Fixes #504)#506Karthik Nadig (karthiknadig) wants to merge 8 commits into
Conversation
Pin comparisons to the exact PR base, fail invalid or regressed performance and coverage snapshots, and harden the E2E JSONRPC client against buffered-output loss and stderr backpressure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Capture existing RefreshProgress notifications in the E2E benchmark and emit deterministic phase and locator percentile distributions so cold-tail latency can be attributed before product changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Classify existing timeout warnings into privacy-safe categories in the E2E benchmark so the macOS cold tail can be tied to exact fallback probe counts without exposing paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Map Apple /usr/bin/python3 aliases through the active developer directory, remove duplicate Xcode and CommandLineTools probes, and skip unresolved shims before the spawn fallback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR targets the macOS cold-refresh ~30s tail latency by avoiding duplicate serialized spawn-based resolution of Apple’s /usr/bin/python3 shim. It adds filesystem-based mapping from the shim to the active developer toolchain Python (Xcode / CommandLineTools) and extends the e2e performance harness to collect/aggregate RefreshProgress telemetry and privacy-safe interpreter timeout categories.
Changes:
- Add macOS-specific utilities to detect and resolve
/usr/bin/python3(andpython3.<minor>) shims to the active developer directory without spawning Python. - Update macOS Xcode and Command Line Tools locators to reuse the file-based mapping and add
/usr/bin/python3as an alias only when it truly matches the discovered interpreter. - Enhance the e2e performance test harness to collect
RefreshProgresstelemetry, assert phase coverage per iteration, and classify interpreter probe timeouts into privacy-safe buckets.
Show a summary per file
| File | Description |
|---|---|
| crates/pet/tests/e2e_performance.rs | Collects RefreshProgress notifications, aggregates phase/locator timing stats, and classifies interpreter spawn timeouts into privacy-safe labels for CI metrics. |
| crates/pet/src/locators.rs | Skips spawn-based resolution for unresolved macOS /usr/bin/python3* shims during fallback resolution to avoid deterministic timeout tails. |
| crates/pet-python-utils/src/macos.rs | New filesystem-based resolver for macOS system Python shims (developer dir selection + alias handling) without spawning. |
| crates/pet-python-utils/src/lib.rs | Exposes the new macos module. |
| crates/pet-mac-xcode/src/lib.rs | Resolves /usr/bin/python3* inputs via file mapping and adds /usr/bin/python3 alias via the shared helper instead of spawning. |
| crates/pet-mac-commandlinetools/src/lib.rs | Same as Xcode locator: file-based shim mapping + shared alias injection, removing spawn-based probing of /usr/bin/python3. |
Review details
Tip
Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Performance Report (Linux)Result: ✅ Within regression budgets
|
Performance Report (macOS)Result: ✅ Within regression budgets
|
Performance Report (Windows)Result: ✅ Within regression budgets
|
Test Coverage Report (Linux)Result: ✅ Within regression budget
|
Test Coverage Report (Windows)Result: ✅ Within regression budget
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (1)
crates/pet/tests/e2e_performance.rs:418
interpreter_probe_timeout_labels()derives timeout counts by scanningstderr_tail, but that tail is explicitly bounded (STDERR_TAIL_LINES = 100). If PET emits >100 stderr lines after a spawn timeout, the timeout warning can be evicted from the tail and the test will undercount (potentially masking a regression).
Consider tracking timeout labels/counts in spawn_stderr_reader as lines are read (store only matching labels/counters, which should stay small) instead of scanning the bounded tail for matches.
fn interpreter_probe_timeout_labels(&self) -> Vec<&'static str> {
self.stderr_tail
.lock()
.expect("PET stderr tail mutex poisoned")
.iter()
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Eliminate the deterministic macOS cold-refresh tail caused by duplicate serialized probes of Apple's
/usr/bin/python3shim. Resolve active developer Python executables from filesystem state instead, while preserving legacy/usr/bin/pythonbehavior.Changes
DEVELOPER_DIR,xcode_select_link, and standard developer-directory fallbacks without spawning PythonValidation
Fixes #504