Repository navigation
Conversation
|
Size Report
Startup median (7 runs, lower is better):
|
7199739 to
1776316
Compare
|
Thanks for the work here. At 7199739 the hang detection reports a false, trace-long hang, so this needs a fix before merge. The The only live run with the tool installed shows the merged window. No run shows an idle trace yielding zero hangs, or induced hangs reported as separate windows. After the fix, please run Not blocking, and you can take or leave these: frame-timeline rows at line 224 carry no state split, yet Could this PR be smaller? Its own commit is about 340 production lines. One SQL statement for main-thread non-idle windows (exclude At 7199739 the PR had merge conflicts with its base, and the rebase above should clear them. The Coverage check is likely related to this PR: it flags A newer head, 1776316, arrived while I was reviewing. It adds tests in |
|
Reviewed 1776316: the main-thread hang rule still reports false hangs, so this needs one more fix. CI is green (6 checks, none failing), and the earlier Coverage CRAP failure on runTraceProcessorQuery is covered by the new mocked-runCmd tests. There are no conflicts. The window rule in perf-trace-processor.ts:129 is unchanged from my earlier comment. It merges every main-thread The tests at perf-trace-processor.test.ts:76 add route coverage, but the existing test still asserts the merged 18s run (17,997.8ms, 6,002.1ms blocked) as a correct hang. It pins the defect and would fail on the right fix. Please replace it with two fixtures: an idle main thread that yields no hangs, and induced busy hangs that yield K separate rows with running-dominant splits. This is a device-facing capture path, and the only live run predates any fix and shows the merged 20s window. After the SQL fix, please run Could a single SQL statement do all of this? It would yield main-thread non-idle windows (exclude Not blocking, take or leave: drop the test-only I could not run trace_processor or the typecheck and test gates here. This review rests on Perfetto's documented contiguous |
…at perf trace stop One fixed SQL statement owns the window rule: a hang is a maximal run of non-idle main-thread thread_state rows (S and unknown states close a window and never count), Running is running time, R/R+/D is blocked time, and the 250ms threshold is filtered in SQL. trace_processor_shell is preferred over trace_processor on PATH; without the tool the stop result keeps the artifact and adds the typed install note. Review follow-ups from #3926: - idle main threads now yield zero hangs; induced 4s hangs yield separate running-dominant rows (real fixture CSVs from the production query) - frame-timeline pass dropped (no app rows on API 35, fake split, double count) - hangs emitted once on summary.hangs; snapshot mirror and traceAnalysis duplication removed - resolveTool seam deleted; runTraceProcessorQuery covered through the host-kit command seam - main thread resolved by joining thread to process (restart-safe) - unexpected analyzer throws map to typed trace-processor-failed - Android summary uses the shared PerfTraceAnalysis envelope
1776316 to
48b0e1c
Compare
|
Fixed at Blocking — false trace-long hang. The window rule now lives in the one SQL statement: Fixtures. The merged-18s fixture and its test are replaced, not kept. Two new fixtures are the production query's own CSV output, produced by running the exact SQL emitted by Live runs (both pasted in the PR body, head Single-SQL question — yes, done. One statement ( Non-blocking, all taken:
|
|
I found no code problems in 48b0e1c, and I could not confirm it end to end. The earlier findings from #3926 (comment) are fixed, and the Coverage CRAP failure on I could not run The checks on this commit were cancelled, not failed, so no job has produced a result for it. The diff touches Lint/Typecheck, Coverage (CRAP of touched functions) and Integration routes. Please re-run the checks and get them green before merge. There are no conflicts. One design question I would like answered before merge: Apple's stop summary spreads the Not blocking, and you can take or leave these: (1) The idle and hangs fixtures in perf-trace-processor.test.ts would also pass with the old merged-window SQL, so only the substring assertions pin the window rule. Running |
…at perf trace stop One fixed SQL statement owns the window rule: a hang is a maximal run of non-idle main-thread thread_state rows (S and unknown states close a window and never count), Running is running time, R/R+/D is blocked time, and the 250ms threshold is filtered in SQL. trace_processor_shell is preferred over trace_processor on PATH; without the tool the stop result keeps the artifact and adds the typed install note. Review follow-ups from #3926: - idle main threads now yield zero hangs; induced 4s hangs yield separate running-dominant rows (real fixture CSVs from the production query) - frame-timeline pass dropped (no app rows on API 35, fake split, double count) - hangs emitted once on summary.hangs; snapshot mirror and traceAnalysis duplication removed - resolveTool seam deleted; runTraceProcessorQuery covered through the host-kit command seam - main thread resolved by joining thread to process (restart-safe) - unexpected analyzer throws map to typed trace-processor-failed - Android summary uses the shared PerfTraceAnalysis envelope
48b0e1c to
e00de8e
Compare
|
Second-pass items handled at Design question — done as suggested. The Non-blocking (1) — fixtures now discriminate against the old merged-window SQL. Agreed that the CSV fixtures are inputs to the interpreter, so only the substring assertions pinned the rule. There is now a rule-level test that runs Non-blocking (2) — Non-blocking (3) — duplicated doc-comment line removed from End-to-end re-verified at this head (since you could not confirm it): both emulator runs re-run after the envelope spread, outputs quoted in the PR body at |
|
This PR is ready at e00de8e. The envelope is now adopted, and |
…at perf trace stop One fixed SQL statement owns the window rule: a hang is a maximal run of non-idle main-thread thread_state rows (S and unknown states close a window and never count), Running is running time, R/R+/D is blocked time, and the 250ms threshold is filtered in SQL. trace_processor_shell is preferred over trace_processor on PATH; without the tool the stop result keeps the artifact and adds the typed install note. Review follow-ups from #3926: - idle main threads now yield zero hangs; induced 4s hangs yield separate running-dominant rows (real fixture CSVs from the production query) - frame-timeline pass dropped (no app rows on API 35, fake split, double count) - hangs emitted once on summary.hangs; snapshot mirror and traceAnalysis duplication removed - resolveTool seam deleted; runTraceProcessorQuery covered through the host-kit command seam - main thread resolved by joining thread to process (restart-safe) - unexpected analyzer throws map to typed trace-processor-failed - Android summary uses the shared PerfTraceAnalysis envelope
e00de8e to
ea1013b
Compare
|
Taken at Unavailable CLI line now prints the hint. Live evidence in the body stays at |
|
Closing this one. Interpreting a local Perfetto trace is something a capable agent can already do with The branch stays up for reference. The |
What
perf trace stop(Android Perfetto) now interprets the finished.perfetto-traceon the host and spreads the trace-analysis envelope onto the stop summary: hangs (start offset, duration, running/blocked split, rule-basednextSteps),hangCount, and notes — the same placement Apple's stop summary uses. Interpretation uses the hosttrace_processortool (trace_processor_shellpreferred on PATH); without it the capture result stays intact and the summary carries the typed unavailable envelope. Builds on the shared contracts shapes from #3925.How
thread_staterows whose state is non-idle. Idle sleep (S) and unknown states close a window and never count toward duration or blocked time. OnlyRunningcounts as running;R,R+,Dcount as blocked. Windows below 250ms (the Time Profiler "Hangs" default, matching Apple's threshold) are filtered in SQL, so the threshold has exactly one site.trace_boundsrides along in the same CSV block for start offsets.thread JOIN process USING (upid) WHERE thread.tid = p.pid AND process.name = <package>— restart-safe, no arbitrary-pid scalar subquery.running = duration, blocked = 0), and double-counted rows overlapping a run. Symbol attribution stays simpleperf's job; Android hotspots are an empty list in the envelope.PerfTraceAnalysisenvelope spreads onto the stop summary exactly like Apple's (notes: [frameNote, ...envelope.notes]), sohangs,hangCount,hotspots, and the unavailablereason/message/hinteach appear once at summary level. There is nosummary.traceAnalysisfield, no snapshot mirror, and the formatter's only unavailable read issummary.available === false— shared with Apple.trace-processor-failedreason instead of vanishing through.catch(() => undefined).trace_processoris never bundled;runTraceProcessorQueryruns a temp SQL file through the host-kit command seam (mockable), with a fixed timeout and typed failure detail from stderr.Ssemantics: idle covers lock/binder/GC waits (those stretches count as idle), and a busy hang with a briefSsplits into two windows.Live device evidence
At head
e00de8e1(current headea1013b6only appends the install hint to the unavailable CLI line; the quoted runs are unchanged by it): API 35 emulator (perfReport35, google_apis arm64), daemon started withtrace_processor_shellon PATH, hang app inducing three 4s busy main-thread hangs at +3/+10/+17s.Idle app (settled, traced while idle):
No
Hangs (section — the old rule reported a 20s false hang here.Hang app (fresh relaunch, all three induced hangs inside the window):
The three induced hangs come out as separate ~4s running-dominant windows. The 4th row is real scheduler behavior, not a merge artifact: the third spin loop's final stretch contains a short idle moment, so the window correctly splits at ~+20s (the old rule would have swallowed it into one trace-long run).
JSON shape at this head (idle-window stop):
summarykeysavailable, capture, frameHealth, hangCount, hangs, hotspots, notes, tracePath— the envelope spread, nosummary.traceAnalysis, notes =[frame note, ...envelope notes].Tests
buildAndroidTraceHangsSqlruns against an in-memorynode:sqlitedatabase shaped like Perfetto'sprocess/thread/thread_state/trace_bounds, asserting thatScloses and never stretches windows, onlyRunningcounts as running, and sub-threshold windows drop — so a reintroduced merged-window rule fails this test even though the committed CSVs are inputs, not SQL outputs, to it.trace_processor(v57) CSV output over fresh captures on this emulator:perf-trace-processor-idle.csv(settled app -> bounds row only -> zero hangs) andperf-trace-processor-hangs.csv(three induced 4s hangs -> three separate windows, running-dominant, ~4000ms each). The old fixture/test that pinned the merged 18s run are replaced.LAG-based window start, join-based main-thread lookup, single threshold site, no frame-timeline table).runTraceProcessorQuerycoverage through the host-kit command seam (no production test seam): tool resolution order (trace_processor_shellpreferred when both resolve), SQL file content, query args, timeout option, typed failure on non-zero exit / throw / setup failure (temp dir cleaned), malformed CSV tolerated while staying available.hangs/hangCountonce + notes; unavailable:available:false+ typed reason), analyzer throw ->trace-processor-failed, non-trace modes unaffected.Validation
pnpm check:affected --run --head ea1013b6f3612adf291655345179dfb350926047-> passed (format, lint, typecheck, layering, di-seams, fallow, mcp-metadata, build, package, integration-node, vitest-related, repo guards; receiptexactCommit=true). Prior heads48b0e1c2ande00de8e1were also gate-green; CI ran 19/19 green on48b0e1c2(an earlier batch there was cancelled, not failed).Depends on #3925 (shared contracts shapes); restacked on the force-pushed base.
🤖 Generated with Claude Code