Repository navigation
Conversation
|
Size Report
Startup median (7 runs, lower is better):
|
8c42c68 to
3b3eb44
Compare
|
I reviewed 3b3eb44 and found three problems to fix before merge. CI is green (22/22 checks pass) and I know of no conflicts. I did not re-run the live simulator capture; I relied on the PR transcript, and its code matches head. The unavailable reason can name the wrong cause. In perf-trace-analysis.ts, any time-profile export failure, including the 15s export timeout on a long trace, becomes The hang breakdown can be inconsistent. classifyAppleHangWindow rescans all samples within 50ms of the run, so neighbouring work is counted as part of the hang. The PR's own output shows a 294ms hang with "running 334ms / blocked 0ms", and Hitch rows are not filtered by process. analyzeAppleHitches counts every row in the hitches table, but that table also carries other processes' rows. Not blocking, and fine to take or leave: (1) the Could this be smaller? Production net is 707 lines. One shared shape in contracts would remove the extra readers: Before merge, please fix the export-failure reason, the hang split and the hitch filter, and answer the shared-envelope question so #3926 can build on it. I did not exercise physical-device behavior or measure whether real time-profile exports pass the 15s timeout. |
f309086 to
729dc23
Compare
|
All findings fixed at Blocking 1 — wrong unavailable cause on export failure. Fixed in Blocking 2 — inconsistent hang breakdown. Blocking 3 — hitch rows not filtered by process. Non-blocking (1) — silent Non-blocking (2) — Non-blocking (3) — JS-thread rule. Dropped from contracts along with its test, as suggested. Non-blocking (4) — Non-blocking (5) — sequential export timeouts. One overall 45s analysis deadline wraps the export + analysis phase ( Non-blocking (6) — hitches test could not catch the missing filter. Rewritten to use the fixture described above (real rows + foreign row), which fails if the filter regresses. Non-blocking (7) — commands.md. Fixed: the nextSteps claim now says each hang carries bounded guidance (CLI prints at most five per list, JSON carries the full list), and the docs state that Shared envelope question — agreed and adopted. Live simulator evidence re-run at this head; output quoted in the PR body with the sha. |
|
This PR is ready at 729dc23. It fixes the problems from the earlier review, all 22 checks pass, and there are no conflicts. Nothing else must happen before merge. Not blocking, and fine to take or leave: (1) in perf-trace-analysis.ts the run end should never move backwards, so use the max of the old end and the new sample end, which keeps running plus blocked time within the run duration; (2) in perf-xctrace.ts the fixed 45s analysis deadline follows a stop wait of up to 50s, so the 90s command envelope can fire first, and the abandoned exports keep running, so the budget could come from the remaining envelope and be passed into the exports; (3) in perf-trace-analysis.ts |
729dc23 to
dc9f004
Compare
|
All three notes taken, at
Live simulator evidence in the body is from the previous head; the shape the fresh evidence printed is unchanged by these three edits (two are structural guards, one removes a redundant filter). |
|
This PR is ready at dc9f004. It fixes what the earlier review at 729dc23 covered, and the code looks correct. All 22 checks pass, and I know of no conflicts. Live simulator evidence is still from 729dc23, not dc9f004. I accepted it because the change since then only lowers or keeps the analysis budget, changes run end only for non-monotonic samples, and removes a redundant filter, so the output shape cannot change. I did not rerun it, and I did not check perf inside batch end to end. Not blocking, take or leave: the 90s Nothing else must happen before merge. |
dc9f004 to
ad3c448
Compare
… trace stop Attach a trace analysis summary to xctrace trace stops: main-thread hangs derived from contiguous Time Profiler samples, hitch tallies from a real hitches table counted only for the attached process, CPU hotspots, and static nextSteps rules. Reasons are typed (table-export-failed names an export failure instead of sending the agent to re-record; analysis-timeout and analysis-failed replace silent drops); analysis runs under one overall deadline and never fails the capture. Contracts hold the shared PerfHang shape, the PerfTraceAnalysis stop-summary envelope, and the static rule table that Android reuses.
ad3c448 to
2e8b2f4
Compare
|
All four notes taken at
Body's evidence header now names this head and the new receipt; the quoted simulator run stays at |
|
I reviewed 2e8b2f4 against the earlier review (#3925 (comment)). The new analysis budget is in place, but one timing path can still start an export with no timeout, so this needs one more change before merge. Dropping the 100ms floor lets The rule should be: no analysis export starts without a positive timeout. Please enforce it once in Could the delta be simpler? It adds a deadline guard at each call site. If the "positive timeout or do not spawn" rule lives in Not blocking, and fine to take or leave: the per-table Checks are still running: Smoke Tests is in progress and has not failed. This update changes only the budget and export code in On evidence: the live simulator transcript is still from 729dc23. This update changes only budget timing and export guards on the stop path, not the output shape, so I accepted it as before and did not rerun it. I did not run the tests or the mutations. I judged the new tests by reading them. I did not reproduce the narrow window above. I confirmed the mechanism by reading Before merge, please guard the TOC export against a timeout of 0 or less, ideally in |
|
Closing this. The analysis runs on a local One result is worth keeping for later: on the iOS 27 simulator, Instruments' |
What
perf trace stop(Apple xctrace) now interprets the finished.traceand attaches asummaryto the stop result: hangs (start offset, duration, thread, top symbol, running/blocked split), animation hitches, CPU hotspots, and rule-basednextSteps. Also makes the defaultperf tracetemplate "Time Profiler" so the hang evidence exists in the capture (the old default, "Animation Hitches", produced malformed traces on simulators under Xcode 27.1).How
hang-risks,hitches, andtime-profiletables viaxctrace exportunder one overall analysis deadline.time-profilesamples (gap bridge = 4x sample weight, 250ms threshold matching the Instruments hangs default). The main thread is identified by theMain Threadfmt label, so no thread-info export is needed. The running/blocked split and top symbol accumulate per run from exactly the samples that formed it, counting only each sample's uncovered exposure, sorunningMs + blockedMs <= durationMsholds by construction.potential-hangsis deliberately not parsed: no capture in this environment (iOS simulator or macOS host, Xcode 27.1) ever fills it — zero rows even for 4s busy hangs — and we don't ship parsers tested only against invented fixtures. A follow-up can add it once a physical-device capture provides a real fixture.parseAppleHitchRows(perf-frame) and only count rows of the attached process (pid or process-name match, incl. bundle-id-shaped targets), so other processes' rows in a shared export are dropped.nextStepscome from a static rule table in contracts (no LLM): running-dominant + symbol -> inspect symbol + CPU profile; blocked-dominant -> move waiting work off main.toc-export-failed,table-export-failed,no-time-profile-table,no-process-samples,analysis-timeout,analysis-failed).table-export-failednames the failed export (the hint does not send the agent to re-record with a template the trace already used);no-time-profile-tableis kept only for a TOC that truly lacks the table. No new CLI actions or flags.PerfHangshape, thePerfTraceAnalysisstop-summary envelope ({available:true, tracePath, hangs, hangCount, hitches?, hotspots, notes} | {available:false, reason, message, hint}) that feat(perf): interpret Android Perfetto traces into main-thread hangs at perf trace stop #3926 builds on, and the static rule table.await importso the perf-facade eager closure stays at its merge-base budget.Output bound
CLI text prints at most 5 hangs/hotspots plus a "see the JSON summary" overflow line;
--jsoncarries the full list.Live device evidence
Built CLI at
729dc234(headsdc9f004eand2e8b2f4dadd only review-pass fixes: monotonic run end, stop-wait-derived analysis budget, dropped redundant matcher, and tests; the printed shape and values cannot change, gate-green at2e8b2f4d), iOS simulatoriPhone 17e(iOS 27.0, Xcode 27.1) with a hang app inducing busy main-thread hangs:Every hang's
running + blockednow sits inside its own duration (the old 294ms/334ms inconsistency is fixed by the per-run accumulation).Honest simulator notes: the derived-samples path above is the one the live evidence exercises (it works on simulators and devices alike); the hitches table is absent from Time Profiler traces and reports
undefined(no evidence) in this run.Tests
perf-trace-analysis.test.ts: derived hang windows + therunningMs + blockedMs <= durationMsinvariant on the real fixture, hitches counted only for the attached process (real export rows plus a foreign-process row in the identical cell encoding — every multi-process export attempt on this host crashedxctrace, so the foreign row itself is constructed, documented in the fixture header), typed unavailable paths incl.table-export-failed, target matching.perf-xctrace-trace-stop.test.ts: stop attaches interpreted hangs through the table mapping (pins a real bug found in live verification: table keys must map to analyzer params), andtable-export-failed+ no-template-suggesting hint when the TOC lists the table but the export fails.Validation
pnpm check:affected --run --head 2e8b2f4d8a74a67a7bd728c1b585f3737c92b763-> passed (format, lint, typecheck, layering, di-seams, fallow, mcp-metadata, build, package, integration-node, macos-coverage, vitest-related, repo guards; receiptexactCommit=true). Prior heads729dc234anddc9f004ewere also gate-green and CI-complete.🤖 Generated with Claude Code