fix(ios): parse the Xcode 27 <tagged-backtrace> time-profile stack - #2889
Conversation
Xcode 27 renamed the time-profile stack element and its engineering-type to tagged-backtrace while keeping the same frame children and id/ref reuse. The parser matched only <backtrace>, so every row of an Xcode 27 export resolved no stack, and writeAppleXctracePerfReport failed a trace holding tens of thousands of samples with "Apple xctrace CPU report contained no samples". Read either spelling through one name set so id/ref resolution, the innermost-frame rule, and the report shape stay exactly as they were for older exports. The fixture is a verbatim 22-row prefix of an xctrace 27.1 (27A9269) export: 19 stack definitions, 2 rows naming their stack through a tagged-backtrace ref, 4 innermost frames and 3 innermost binaries reached through refs, and one row whose stack column is a sentinel. The multi-run test now carries a cross-run frame ref because a real append-run export numbers ids with one counter across its runs. Closes #2860
The package-capture ownership rule matched .json only, so the first recorded xctrace export under a package fixture directory had no derivable owner and failed the gate open to all 59 checks even though exactly one suite reads it. Accept .xml beside .json and pin both spellings in the ownership test.
Size Report
Startup median (7 runs, lower is better):
|
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Thermo-nuclear structural pass. The production change is genuinely good and I would take it on its own: the APPLE_TIME_PROFILE_STACK_ELEMENT_NAMES Set honestly expresses "either spelling of the stack element", the backtrace -> stack rename drops the now-false singular, perf-xml.ts is untouched, and the findAllXmlNodes ~130k RangeError is pre-existing and not made newly reachable by this diff (traversal was already document-wide). No file crossed 1k.
Requesting changes for one reason only: the committed fixture leaks personal + corporate identity. Fix that and this is approvable. One minor below it.
Minor, non-blocking (not inline): the rewritten aggregates rows exported from multiple trace runs through one document-wide id index test pins a real (verified: indexXmlNodesById / findAllXmlNodes are document-wide) but pre-existing cross-<node> ref property — off-topic for a tagged-backtrace fix — and it retires the original independent-per-run aggregation case it replaced. Consider a separate focused test rather than repurposing the multi-run aggregation test.
|
|
||
| test('reads stacks from a real Xcode 27 tagged-backtrace export', () => { | ||
| const xml = readFileSync( | ||
| path.join(import.meta.dirname, 'fixtures', 'xcode27-time-profile.xml'), |
There was a problem hiding this comment.
BLOCKER — this fixture is verbatim, unscrubbed xctrace output from your machine and should not land as-is. It embeds author + corporate identity: your real home dir / username (/Users/michal/..., twice), your per-user var/folders/<hash>/T temp token, and installed-software identity (io.kandji.KandjiAgent.ESF-Extension — an MDM / endpoint-security agent — plus dtsecurity, opencode, fff-scan). Those path= / fmt= values carry zero assertion value and permanently publish personal + employer/security posture into a public repo. The repo's other captured fixtures (e.g. wire-vocabulary.json) are scrubbed; this one is not.
It is also near-redundant: the new synthetic resolves tagged-backtrace and frame references and skips stack sentinels test already covers tagged-backtrace inline + ref, <frame ref>, <weight ref>, and sentinels, and aggregates weighted innermost frames already covers <binary ref> resolution — which was the only unique coverage the real export claimed. A ~30 KB / 24-line near-raw blob (lines up to 2000 chars) is the worst fixture shape: it defeats diff, format, and grep for no behavioral gain.
Either drop it and keep the synthetic test (add a <binary ref> case if you want that explicit), or keep a real-shaped fixture scrubbed to synthetic paths / process names and trimmed to a few representative rows. Please do not merge near-raw machine output.
There was a problem hiding this comment.
Fixed in 42b3d97 — the capture is gone and this PR ships no fixture. The shapes it proved now sit in the synthetic rows with synthetic symbols and binaries: a <tagged-backtrace> definition, a <tagged-backtrace ref>, a truncated="YES" stack whose innermost frame is a <frame ref>, an innermost <binary ref>, and a stack-column <sentinel/> row.
Taking the first option also retires the check-affected .xml ownership rule (9dd0837), since nothing non-.ts lands anymore — the plan now selects 13 checks instead of failing open to 59.
| owns: (file) => | ||
| file.startsWith('packages/') && | ||
| file.endsWith('.json') && | ||
| (file.endsWith('.json') || file.endsWith('.xml')) && |
There was a problem hiding this comment.
The extension list now lives twice: this (endsWith('.json') || endsWith('.xml')) chain, and verbatim in the block comment directly above (fixtures/*.json or *.xml). The next capture format means editing both and they will drift — classic "list wants to be data". Can we lift one const PACKAGE_CAPTURE_EXTENSIONS = ['.json', '.xml'] as const used as .some((ext) => file.endsWith(ext)), and reword the comment to "a known payload-capture extension" without re-listing? If two entries do not earn a constant, at least drop the duplication in the comment.
There was a problem hiding this comment.
Dropped in 9dd0837 rather than de-duplicated: the rule only widened for the capture this branch no longer commits, so it would own nothing real and its test would pin a path that does not exist. model.ts is back to exactly origin/main. If you want the extension list anyway for the next recorded capture, I would take it as its own change with a capture behind it.
…tting a capture The captured export leaked a machine's home directory, per-user temp-dir token, and installed-software identity — an MDM endpoint-security agent among them — into a public repo, and a 30 KB blob of 2000-char lines defeats diff, format, and grep for coverage the synthetic rows already give. Keep the shapes that capture proved: a `<tagged-backtrace>` definition, a row that names its stack by ref, a `truncated="YES"` stack whose innermost frame is a `<frame ref>`, an innermost `<binary ref>`, and a stack-column `<sentinel/>` row. The binary-ref case is the one the pre-existing suite survived before. Symbols, binaries, and weights are synthetic. Give the cross-run frame reference its own test, so the multi-run aggregation case keeps answering independently the way it did before this PR.
That extension was added only to own the captured xctrace export this branch no longer commits, so the rule would own nothing real. Revert to the `.json` capture rule and keep the ownership test on the captures that exist.
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Applied the non-blocking point in 42b3d97: aggregates rows exported from multiple trace runs is restored to the version on main, and the cross-run reference moved to resolves a frame reference an earlier trace run defined.
Checked that the new test is the only one carrying that claim — replacing the document-wide index with one that cannot see the earlier run's ids trips exactly that test and leaves the other four green.
Rebased evidence for the reworked tests is in the Validation section; pnpm check:affected --run is green at 9dd0837.
|
Reviewed at 9dd0837. The code looks good: the parser now reads both Not blocking: the comment at I did not repeat the live Xcode 27 capture. The sample counts in the PR body are your reported result, and the fixture rows are synthetic because the original capture was removed. Smoke Tests, Repo Guards and Coverage were still running at review time. |
thymikee
added
the
ready-for-human
|
1 participant
Summary
Xcode 27 exports every
time-profilesample stack as<tagged-backtrace>; the parser read only<backtrace>, so each row resolved no stack andperf cpu profile report --kind xctracefailed withApple xctrace CPU report contained no sampleson traces holding tens of thousands of samples. Bothspellings now resolve through the same
id/refpath, so the innermost-frame rule, older Xcodeexports, and the report shape are unchanged. Three files, no committed capture per review.
Closes #2860
Validation
9dd083741:pnpm check:affected --run— 13 selected checks, all runnable ones passed. Nothingnon-
.tslands, so the plan no longer fails open to the full set.<tagged-backtrace>definition, a<tagged-backtrace ref>, atruncated="YES"stack whose innermost frame is a<frame ref>, aninnermost
<binary ref>, and a stack-column<sentinel/>row. Mutations that now fail a test: thepre-fix parser, tagged-only matching, last-frame-as-innermost, unresolved stack/frame/binary refs,
and an id index that cannot reach an earlier run's definitions. The
<binary ref>case is one theprevious suite survived.
com.callstack.agentdevicelab:perf cpu profile start --kind xctrace --out ios27.trace, thenstop, thenreport --kind xctrace --out report.json→sampleCount: 13,totalSampleWeightMs: 13, top entry__sysctl | libsystem_kernel.dylib | 3 ms | 23.1%. That traceexports 13
<tagged-backtrace>and 0<backtrace>. Session closed, app uninstalled, simulatorshut down.
findAllXmlNodesthrowsRangeErrorpast ~130k rows.