chore: add Phase 4.1 performance evidence - #687
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: daff52f4bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (heapProfiler) { | ||
| await heapProfiler.stop(); |
There was a problem hiding this comment.
When PERF_HEAP_PROFILE_PATH points to a missing or unwritable directory, heapProfiler.stop() rejects during the asynchronous timer callback, so the completion log and app.quit() are never reached. The fixed-duration performance run then remains alive indefinitely and also produces an unhandled rejection; catch and log the export error while placing app.quit() in a finally path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a32f50a2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ? new PerfHeapProfiler(heapProfilePath) | ||
| : undefined; | ||
| if (heapProfiler) { | ||
| await heapProfiler.start(); |
There was a problem hiding this comment.
When the inspector rejects connect, enable, or startSampling—for example because another sampling profiler is active—this awaited call rejects before the fixed-duration timer is registered. The performance run then never reaches its configured completion, and observer mode remains alive indefinitely; catch startup failures and ensure the duration timer is scheduled regardless.
Useful? React with 👍 / 👎.
| await heapProfiler.start(); | ||
| log.info(`[PerfRun] Main-process heap sampling started`); | ||
| } | ||
| if (perfRun.durationSeconds > 0) { |
There was a problem hiding this comment.
When PERF_HEAP_PROFILE_PATH is used with the standard npm run perf:run default of no --duration-seconds, durationSeconds is zero and this branch is skipped. Sampling starts, but stop() and the profile write exist only inside this branch; stopping the runner with Ctrl+C terminates the app without producing the requested heap profile. Either require a fixed duration when profiling or add a shutdown path that exports the profile.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05922a9e27
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| log.info(`[PerfRun] Main-process heap profile written`); | ||
| } | ||
| } catch (error) { | ||
| log.error(`[PerfRun] Main-process heap profile export failed`, error); |
There was a problem hiding this comment.
When analytics is enabled and profile export fails—for example, because PERF_HEAP_PROFILE_PATH names a missing directory—the Node filesystem error contains the verbatim path, and Analytics.setupLogTransport() forwards error-level log arguments to PostHog. Logging the raw error here therefore discloses a potentially user-identifying home-directory path; sanitize the path prefixes or log only non-path error fields.
AGENTS.md reference: AGENTS.md:L14-L18
Useful? React with 👍 / 👎.
1 participant
Description
Completes the Phase 4.1 packaged Windows performance evidence gate described in
docs/IMPLEMENTATION_PLAN.md.The final Phase 3 build (
6fbc334) and current main (28cbf61e) were compared with packaged builds, the same deterministic 40-car replay tape, dashboard/settings, three-display 7680x1440 geometry, and default Chromium flags. Each full-dashboard capture ran for 420 seconds with a 60-second analysis warm-up.CPU and responsiveness remain stable:
processTelemetryp99: 1.87 ms to 1.94 msThe memory gate remains open. Main-process private-memory growth reproduced at +5.60 MB/min versus the previous Phase 4 result of +5.82 MB/min.
Because retention remained significant, this adds benchmark-only main-process V8 heap sampling controlled by
PERF_HEAP_PROFILE_PATH. The matched retention capture sampled 2.50 MiB of live allocations. Logging serialization accounted for 52.5%, SDK parsing 18.2%, timers 16.0%, and processor registry/processors only 6.3%. The evidence does not support processor snapshots, reusable buffers, channel publication, or subscription ownership as the primary retention owner. Follow-up should isolate logging/transport buffers and external/native or reserved V8 memory before proposing processor fixes.The strict comparison analyzer reports the A/B as inconclusive because the historical Phase 3 artifact predates the current capture-origin/scenario/widget-input metadata and the replay does not exercise event-driven lap-time/radio publications. The matched raw metrics and this limitation are documented explicitly.
Phase 4 is now recorded as delivered and validated with its memory gate open. This PR does not begin Phase 5 or Phase 6.
Screenshots
Before
Not applicable — no visual changes.
After
Not applicable — performance evidence, documentation, and benchmark-only instrumentation.
Type of Change
Checklist
npm testnpm run lintand fixed any issuesValidation
tsc --noEmit: passedgit diff --check: passednpm run lint: blocked by the existing nestedtmp-data/irsdk-replay-worktree, which creates unrelated TypeScript ESLint parser-root errors