RUM-18135: Add a cold-start benchmarking harness and methodology guide - #3749
RUM-18135: Add a cold-start benchmarking harness and methodology guide#3749Valpertui wants to merge 14 commits into
Conversation
|
✅ All CI checks and tests passed. Datadog automation helped this PR pass. 🎉 All green!🧪 All tests passed 🔄 Datadog retried 2 tests - 2 passed on retry 🎯 Code Coverage (details) 🔗 Commit SHA: e5d34a5 | Docs | View more details | Give us feedback! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcc1de7649
ℹ️ 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".
dcc1de7 to
5e229ec
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e229ec8a7
ℹ️ 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".
5e229ec to
597ffb3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 597ffb303b
ℹ️ 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".
597ffb3 to
4dc3290
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4dc32909e2
ℹ️ 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".
4dc3290 to
652dbe8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 652dbe8e37
ℹ️ 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".
OliviaShoup
left a comment
There was a problem hiding this comment.
thanks for the PR! it looks great. the only must-fix issues are British spellings because our style guide prefers American English (sorry lol)
also flagging: em dashes are used very heavily and consistently throughout all three new prose files. every use is correct, but it's dense enough to read as fatiguing or AI-generated to some readers. worth a look if you want to vary it up, but not a blocker
652dbe8 to
9d89830
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d8983034d
ℹ️ 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".
9d89830 to
7775505
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7775505e82
ℹ️ 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".
7775505 to
f9a1b2e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9a1b2e758
ℹ️ 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".
f9a1b2e to
716d40b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 716d40bfd9
ℹ️ 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".
716d40b to
96d8209
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96d8209f6c
ℹ️ 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".
96d8209 to
cb50eae
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb50eae5a8
ℹ️ 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".
cb50eae to
a8d8fec
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8d8fec2e3
ℹ️ 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".
a8d8fec to
d095f0d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 081ef091ff
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5e0a003eb
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37c315ad3b
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 800f46cfc9
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b7d454638
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 450dfa73fe
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a964771385
ℹ️ 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".
lib.sh holds the helpers the three device-touching workflows share, so they cannot drift into different definitions of the same thing: tool resolution, the Android-user snapshot, the runtime-permission grant, the animation-scale and radio controls, the verified uninstall, cold-launch validation and the datadog-* thread liveness probe. Package state on Android is per-user, so one numeric user is resolved once and passed explicitly to every package, permission and activity command. Every readable setting a caller changes is snapshotted here for its EXIT trap to restore; fixed-performance mode and the background dexopt job have no getter, so those are reversed best-effort and only when the command was issued. .gitignore covers the run artifacts: results_*.csv, bench_*.log and *.pftrace, the last because a Perfetto capture is device-wide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
verify_sdk_active.sh is step zero of the protocol: install the APK, md5-attest the install against the local file, launch through the real launcher intent and prove Datadog.initialize() ran in the launched process. The oracle is the datadog-* thread CoreFeature.initialize() always creates (setupExecutors() then an immediate NTP-sync submit, CoreFeature.kt:265-266), which R8 cannot rename because the name is built at runtime from a string template. It is fail-closed: a thread dump it cannot read is a failure, not a pass. Like the other two workflows it uninstalls and reinstalls the app under test and pre-grants its permissions, so it is as destructive as a benchmark run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
coldstart_bench.sh runs the ABBA-counterbalanced A/B (or A/A) and writes one CSV row per launch. Per install: a verified uninstall, an md5 attestation, and a pre-grant of runtime permissions whose effective outcome is hashed into the CSV and re-checked on every install. Every launch is gated on Status=ok, LaunchState=COLD, a numeric TotalTime, the app owning the foreground for the whole collection window, and the SDK being as live in the measured process as the arm claims. A failed gate aborts the run rather than dropping the sample, because an outcome-dependent drop can manufacture an improvement. The ramp launches, the liveness probe and the warm-up discards are held to the same gates, since they define the cell's post-install state. A preflight refuses a PKG that does not match the APKs, arms declaring different versionCode/versionName, and a missing aapt2. Controls are read back and required to match before anything is measured, since settings put reports success on a device that ignored it. The results CSV and its log are reserved with exclusive creates before the first device command, so two runs started in the same second cannot interleave their evidence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ab_stats.py analyzes a results CSV. The primary endpoint is a paired test over per-block deltas; Welch and permutation are printed as [diagnostic]. Launches inside one arm-by-block cell share an install, an AOT compilation and a thermal state, so an unpaired test estimates the standard error from within-cell scatter only, and counterbalancing does nothing about that. Most of the file is refusals: pooling CSVs from different builds, arms, devices, Android users, protocols or effective runtime permissions; counting the same file twice, including a byte-identical copy; reading an aborted run as a complete one; dropping a launch the harness rejected or a metric missing from some launches, both of which are outcome-dependent. The primary interval is also suppressed when a row omits its validity verdict or a contributing block has no record of which arm ran first. Every override is named and leaves that interval suppressed. An interrupted run is recoverable: recovery keys off zero markers and a consecutive block prefix, and reports over the whole blocks it collected. fp_simulation.py reproduces the false-positive table behind the paired design, calling ab_stats.py's own interval code so the two cannot drift apart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
capture_trace.sh captures one launch under Perfetto with the same runtime controls, liveness gate and whole-window foreground check as the A/B, and binds the trace to the benchmark CSV it is meant to explain rather than to transcribed identities. verify_trace.py checks the capture and states only what its own checks can distinguish. TRACE_ENDPOINT (total_ms default, or ttfd / app_trace_ms) makes the capture prove it reached the endpoint the A/B measured, and fails it otherwise: a trace that stops short of the window under study cannot explain that window. Two operator-visible behaviors: the .pftrace name is checked before any device mutation and again immediately before adb pull, so a capture never overwrites an existing file, and a rejected capture is deliberately left on disk for diagnosis. ALLOW_MISSING_LAUNCH_MARKER covers devices that never emit ActivityManager's global "launching: <pkg>" slice, an implementation detail rather than a contract; the lifecycle-only checks still run and the verdict becomes held-lifecycle-only rather than held. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
128 tests over the cross-script contracts, driven by a fake adb so they need no device. They assert the refusals rather than the happy path: the validity gates and their abort paths, the control read-backs, the pooling and dedup rules, the recovery of an interrupted run, and the trace identity and endpoint gates. python3 -m unittest discover -s tests Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tools/coldstart-benchmark/README.md is the operator's reference for the scripts: the ordered usage, the environment variables and named overrides, what the benchmark does per arm and per block, the CSV header and columns, how to report an interrupted run, how to read the output, and the known limits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Customers ask what the SDK costs at startup, and the answer depends on their app, their device mix and their feature set. docs/benchmarking_sdk_cold_start.md publishes the method instead of a number: build release-configured APKs, use a physical device, prove the SDK initializes, validate the protocol with an A/A run, then measure. It states what the harness measures, a process-cold, page-cache-warm start to first frame, and tabulates which controls bias the result and in which direction. Discarded warm-ups, am force-stop, TTID-only measurement and pre-granted permissions all shrink the number, so the figure is closer to a lower bound than a worst case. Two do not, and the table says so: the default speed-profile compile lands at verify on a fresh install, which is pessimistic, and animations off enlarged the total on the app measured. It also records what the exercise taught us that contradicts the intuition: TTID is the wrong endpoint on framework apps (~630 ms TTID against ~2075 ms TTFD on one React Native app); Session Replay's startup increment was ~9 ms, inside the noise, while the cost was in the core SDK; the largest block of SDK CPU we measured began 406-500 ms after reportFullyDrawn() in all ten traces and is not a startup cost; and tracing itself lengthened the measured window by ~7%, so traces say what work exists and where, not how much it costs. Linked from the root README next to the other troubleshooting guides. The existing sdk_performance.md figures are marked as predating this protocol so they are not read as a comparable baseline; no number there changes here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Same protocol as the guide, written for an agent driving the investigation rather than a human reading it end to end: ordered steps, hard gates, and the measured cost of skipping each control. It front-loads the two gates that catch a plausible number from a broken protocol -- prove the SDK initializes, then A/A the protocol -- and refuses to treat an A/B as meaningful until both pass. It also carries the attribution rules that are easy to get backwards: what each verify_trace.py exit code licenses you to conclude, why a stopped app still passes a liveness check, and why work has to be timestamped against the measurement endpoint before it can be called a startup cost. Ships in the repository so a clone picks it up, and assumes nothing about whether the app being measured is ours or a customer's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b827f9f to
ccaf9d5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccaf9d5eab
ℹ️ 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".
Bind analysis to the recorded arm roles and preserve full device and collector identities across CSV handling. Refuse a header value the header's own whitespace-tokenized format cannot carry, so a value that decides comparability is never recorded truncated to its first word. Classify launcher failures as unverifiable setup and abort a launch when its force-stop precondition cannot be established, the liveness probe included: it runs on the left of a pipeline, where errexit does not apply, so a rejected stop continued to the launch and the pipeline still reported success. Add regression coverage and update the harness documentation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reject failed force-stop and semantically invalid am start output before consulting SDK liveness. Cover the Motorola exit-zero Error type 3 behavior with full-script regression tests and document the verifier contract.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 149f67fcc1
ℹ️ 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".
Four exit-code and reporting contracts, and one reversal of what an interrupted run is allowed to claim. verify_sdk_active.sh reserves exit 1 for "the SDK is not initializing". Two setup failures could borrow it: a remote md5sum that failed under pipefail, and an invalid SETTLE reaching sleep, where `SETTLE=twenty` exits 1 and `SETTLE=--help` returns immediately without waiting at all. Both exit 2 now, and SETTLE is checked before the first device command. The digest is read from the md5sum line for the path that was asked about rather than from the first line of output, so a device that writes to stderr first does not have its warning parsed as the digest and reported as a wrong-APK install. An interrupted run is no longer reportable. It used to emit a primary interval and a significance verdict over its whole counterbalanced prefix, on the argument that the paired estimator is unbiased at any even block count. That covers the bias a shortfall introduces, but not the bias in how the run stopped: rows are visible while collection runs, so a prefix can be chosen after a favourable delta, and an automatic abort can correlate with thermal or storage state that was also acting during the blocks it kept. Nothing in the CSV establishes an outcome-independent stopping rule. The prefix stays inspectable and still prints its shortfall and abort trailer, but the primary CI, MDE and verdict are suppressed. The four-block floor stays for a different reason now, and says so. Restoration no longer claims a success it does not have. Every cleanup command in the benchmark and trace EXIT paths discarded its status, and the operator was told the device was restored either way -- worst on exactly the abort path an adb failure causes. Failed controls are named for manual recovery instead. The collection exit status is unchanged, because a cleanup failure does not retroactively alter samples already recorded. APP_TRACE_REGEX is passed after grep's `--` at the preflight compile test and at every consumer. A pattern beginning with a valid option, `-e[0-9]+`, was taken as that option: the scrape then recorded the last number from the first unrelated timestamped line, and the trace watcher declared the endpoint reached on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0099ed7cd
ℹ️ 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".
Four contracts, and one finding declined. The liveness oracle no longer draws absence from a stale PID set. A package-private process can start while the earlier set is being read, and a zero over that set is not package-wide absence. The oracle re-enumerates after the reads and, if the package gained a process, reads that one too, so a late `<pkg>:private` holding the only `datadog-*` thread is counted rather than missed. Only additions matter: a PID that vanished was already read, or its failed read was already refused by the `/proc` re-check, so treating its exit as unknown would abort an hour-long run whenever a short-lived `:startup` process finished during the read -- the same churn the per-PID loop deliberately tolerates. A package still spawning after a second pass has no coverable snapshot and stays unknown. All four production callers pass the package, and a test holds them to it. Cleanup ownership is dropped once the previous installation is proven absent, before the next APK is installed. A later failure between install and pre-grant -- dexopt attestation, launcher resolution -- otherwise left `_GRANTED` naming the uninstalled generation's grants, and the EXIT trap revoked those names from the new install, taking a permission this run never granted. Clearing after the verified uninstall rather than before it keeps ownership of the old generation's grants for as long as that app is still on the device. Trace capture no longer derives `--expect-ndk` from core SDK liveness. NDK crash reporting is optional configuration the benchmark does not record, so a core-only arm was warned that a native library it never enabled was not observed. Any `libdatadog-ndk` load stays informational, and the docs say to add the flag by hand only when the app enables the feature. The interrupted-run row in the troubleshooting table promised an MDE and an interval that the analyzer suppresses, so it contradicted both the section above it and the tool. It now states the diagnostic-only outcome, and a test reads that row and holds it to what the tool prints. Declined: binding trace capture to the recorded device model. Keeping one physical device across a benchmark and its explanatory trace is the operator's responsibility, not something the harness checks; a model hash would not identify a handset anyway. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88a7831fe3
ℹ️ 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 ($1 !~ /^[0-9]+$/ || NF < 2) exit 3 | ||
| saw_process=1 | ||
| if ($2 == p || index($2, p ":") == 1) print $1 |
There was a problem hiding this comment.
Enumerate processes that use an explicit global name
When a component declares a fully qualified android:process name that is not <applicationId> or <applicationId>:..., this name-based filter omits a process owned by the APK. If Datadog initializes only there, the treatment arm is rejected as inactive; worse, if another conventionally named app process exists, an accidentally SDK-active baseline can pass the absence gate without inspecting the process that owns the SDK thread. Enumerate processes by the installed package UID (failing closed for shared UIDs) rather than assuming every app-owned process uses the package-name prefix; the equivalent name filter in verify_trace.py needs the same treatment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5d34a50e3
ℹ️ 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".
| for _v in RUNS BLOCKS WARMUP; do | ||
| case "${!_v}" in ''|*[!0-9]*) die "$_v must be a non-negative integer (got '${!_v}')" ;; esac | ||
| done |
There was a problem hiding this comment.
Normalize decimal counts before Bash arithmetic
When a caller supplies a zero-padded count such as RUNS=010 or BLOCKS=010, this validation accepts it, but Bash arithmetic interprets it as octal while the Python analyzer parses the stamped header as decimal. The collector therefore records only 8 launches/blocks while declaring 10, writes # RUN COMPLETE, and the analyzer later refuses the expensive run as an incomplete matrix; values such as 08 can instead make the arithmetic loop error and collect nothing. Reject leading zeroes or normalize these inputs to base 10 before using or recording them.
Useful? React with 👍 / 👎.
| if [ "${ALLOW_UNVERIFIED_PKG:-0}" = "1" ]; then | ||
| log "WARNING: ALLOW_UNVERIFIED_PKG=1 -- the APK/package preflight is DISABLED." | ||
| log " 'adb uninstall $PKG' will run against whatever app owns that id." | ||
| else | ||
| check_apk_pair |
There was a problem hiding this comment.
Keep version validation independent of the package override
When aapt2 is unavailable and the operator follows the documented recovery of setting ALLOW_UNVERIFIED_PKG=1 after manually confirming the application ID, this branch skips all of check_apk_pair(), including its separate versionCode/versionName equality check. Baseline and treatment APKs from different app releases can therefore run without ALLOW_VERSION_MISMATCH=1, producing a completed result whose delta is not attributable to the SDK. Require the version-mismatch acknowledgement separately when the override prevents automatic version validation, rather than silently disabling that invariant with the package check.
Useful? React with 👍 / 👎.
| trap cleanup EXIT | ||
| trap 'exit 130' INT | ||
| trap 'exit 143' TERM | ||
|
|
||
| dd_reserve_output_files "$TRACE_FILE" || die "trace output reservation failed" |
There was a problem hiding this comment.
Arm trace cleanup only after reserving the output
When $NAME.pftrace already exists or another process wins the reservation race, dd_reserve_output_files() calls die, but this EXIT trap has already been installed and cleanup() unconditionally writes every snapshotted animation, timeout, stay-awake, and radio value back through settings put/svc. The advertised pre-mutation collision check can therefore mutate the device despite refusing the capture, and can overwrite a setting changed by another actor after the snapshot. Reserve the trace path before arming mutating cleanup, or track whether each control was actually changed before restoring it.
Useful? React with 👍 / 👎.
What does this PR do?
Adds a cold-start benchmarking harness, a customer-facing methodology guide, and an agent skill: the tooling and the written contract for answering "how much does the SDK cost at app startup?" on a given app and device.
Nine commits, purely additive (12 new files, no SDK code touched). Each adds one reviewable unit in its final form; no commit re-touches an earlier one, so the history reads foundation -> prerequisite -> measure -> analyse -> attribute -> prove -> document:
Add shared device controls for the cold-start harnesslib.sh,.gitignoreAdd the SDK-liveness oracleverify_sdk_active.shAdd the A/B cold-start runnercoldstart_bench.shAdd the paired per-block statisticsab_stats.py,fp_simulation.pyAdd optional Perfetto trace attributioncapture_trace.sh,verify_trace.pyAdd regression tests for the harness contractstests/test_harness_regressions.pyAdd the operator reference for the harnesstools/coldstart-benchmark/README.mdDocument how to measure the SDK's cold-start impactdocs/benchmarking_sdk_cold_start.md, linked from the root READMEAdd a coldstart-benchmark skill for coding agents.claude/skills/coldstart-benchmark/SKILL.mdAll nine are prefixed
RUM-18135:. Reviewing commit-by-commit is the intended path; commit 6 is the contract the five preceding commits are held to.The harness:
verify_sdk_active.sh: step zero. Installs, md5-attests the install against the local file, launches via the real launcher intent, and provesDatadog.initialize()actually ran. The oracle is thedatadog-*threadCoreFeature.initialize()always creates (setupExecutors()then an immediate NTP-sync submit,CoreFeature.kt:265-266) and that R8 cannot rename, because the name is built at runtime from a string template.coldstart_bench.sh: the A/B (or A/A). ABBA-counterbalanced blocks, a verified uninstall and md5 attestation per install, pre-granted runtime permissions whose effective outcome is hashed into the CSV and re-checked on every install, a pre-registered warm-up discard, and a per-launch assertion that the launch reportedStatus=ok/LaunchState=COLDwith a numericTotalTime, that the app owned the foreground for the whole collection window, and that the SDK was as live in the measured process as the arm claims. Aborts rather than recording a sample it cannot vouch for. Every launch that conditions the ramp, the liveness probe and the warm-ups alike, is held to the same gates, since those are what define the cell's post-install state.ab_stats.py: statistics. The primary endpoint is a paired test over per-block deltas; Welch and permutation are printed as[diagnostic]. Most of the file is refusals: pooling CSVs from different builds, arms, devices, Android users or protocols; counting the same file twice, including a byte-identical copy; reading an aborted run as a complete one; recovering from a file whose whole blocks are not a consecutive prefix from block 1, which the sequential collector cannot produce; dropping a launch the harness rejected or a metric missing from some launches (both are outcome-dependent, so excluding them can manufacture an improvement). It also refuses to pool runs whose arms ended up with different effective runtime permissions, and suppresses the primary interval when a row omits its validity verdict or a contributing block has no record of which arm ran first. Every override is named and leaves the primary interval suppressed.fp_simulation.py: reproduces the false-positive table that justifies that choice.capture_trace.sh/verify_trace.py: optional Perfetto attribution, with a liveness gate, an endpoint gate and a whole-window foreground check.lib.sh: shared helpers, so the three device-touching workflows cannot drift into different definitions of the same thing: tool resolution, the Android-user snapshot, the permission grant, the animation-scale and radio controls, the verified uninstall, cold-launch validation and thedatadog-*liveness probe.tests/test_harness_regressions.py: 128 regression tests for the cross-script contracts, driven by a fakeadb, so they need no device. They assert the refusals, not the happy path.Motivation
Customers ask what the SDK costs at startup. The honest answer depends on their app, their device mix and their feature set, so the useful thing to publish is not a number but a method they can run, plus the caveats that decide whether the number it produces means anything.
The naive protocol does not work. Running the same APK in both arms, so the true difference is zero by construction, on a mid-range device (Helio G95, Android 12):
The first row is a statistically significant regression between two identical builds. Anyone running that protocol would have reported it.
Two design decisions are worth flagging for review:
The primary endpoint is paired on blocks, not pooled over launches. Launches inside one arm×block cell share an install, an AOT compilation and a thermal state, so an unpaired test estimates the standard error from within-cell scatter only. Counterbalancing removes the ordering bias; it does nothing about this. By simulation (true effect zero, within-launch sd 11 ms, per-cell shift sd σ_b), false-positive rate of a nominal-95% interval:
A 4 ms between-block shift is ordinary.
./fp_simulation.pyreproduces this table usingab_stats.py's own interval code, so it cannot drift away from the tool.Most controls bias the measured cost downward, and the guide says so. Discarded warm-ups,
am force-stop(which does not evict the page cache), TTID-only measurement and pre-granted permissions all shrink the number. Two do not, and the table says which way they go instead: the defaultspeed-profilecompile lands atverifyon a fresh install, which is pessimistic rather than flattering, and animations off enlarged the total on the app measured (+88.6 ms against +40.2 ms on TTFD) despite suppressing per-frame SDK work.docs/benchmarking_sdk_cold_start.mdtabulates the direction of each. The metric is a process-cold, page-cache-warm start to first frame, and is closer to a lower bound than a worst case.Additional Notes
Findings the guide documents that contradict the intuition, each measured rather than assumed:
com.datadog.*JIT (~390 ms with Session Replay), across ten traces. Timestamped: zero of it began beforereportFullyDrawn(); the first started 406–500 ms after, in all ten traces. It is post-launch background CPU. A Baseline Profile would not have moved TTID or TTFD on that app.On
docs/sdk_performance.md: the guide explicitly marks those figures as predating this protocol (5 launches per arm, fixed order, no A/A, no confidence intervals) so they are not mistaken for a comparable baseline. No numbers there are changed by this PR.Safety. All three device-touching scripts uninstall/reinstall the app under test and pre-grant its runtime permissions, including
verify_sdk_active.sh, which is documented as step zero and is just as destructive. Every readable device setting they touch is snapshotted and restored from anEXITtrap;INT/TERMexit into that trap, so Ctrl-C stops the run and restores the device, once. Two controls, fixed-performance mode and the background dexopt job, have no Android getter, so those are reversed on a best-effort basis and only when the script successfully issued the command itself.Package state on Android is per-user, so one numeric Android user is resolved once and passed explicitly to every package, permission and activity command, and stamped into the CSV header. Only permissions found denied are granted, and only those are revoked afterwards, so a grant the app already held is left alone, and
pm reset-permissions, which would drop every other app's grants, is never used. The scripts refuse to run when the package is also installed for another user, because host-sideadb uninstallhas no user selector and would delete that profile's app data.A preflight refuses to run when
PKGdoes not match the APKs (every block runsadb uninstall $PKG) or when the two arms declare differentversionCode/versionName; a missingaapt2is a hard failure rather than a downgrade to a warning..gitignorecoversresults_*.csv,bench_*.logand*.pftrace, since a Perfetto capture is device-wide.No CHANGELOG entry: tooling and docs only; no shipped SDK code changes.
Operator-visible behavior worth knowing before you run it
Three things will change what an operator sees, beyond the statistics:
capture_trace.shnever overwrites a.pftrace. The name is checked before any devicemutation and again immediately before
adb pull. This matters because a rejected capture isdeliberately left on disk for diagnosis, so the file that explains the rejection is also the
file that now blocks re-running the same name. Move it or pick another name. The old behavior
destroyed the evidence silently, which is worse, but the iterate loop is two commands now.
TRACE_ENDPOINT(total_msdefault, orttfd/app_trace_ms) makes the capture prove itreached the same endpoint the A/B measured, and fails the capture when it did not. A trace that
stops short of the window under study cannot explain that window.
ALLOW_MISSING_LAUNCH_MARKERexists because the whole-window foreground check prefersActivityManager's global
launching: <pkg>slice, which is anActivityMetricsLoggerimplementation detail rather than a documented contract. On a device that never emits it, set
this to
1: the lifecycle-only checks still run and the verdict becomesheld-lifecycle-onlyrather than
held, so a capture whose foreign-takeover clause never ran is never reported asclean.
settings putreports successon a device that ignored it, so the three animation scales and both radio settings are read
back and required to match before anything is measured; an app that declares a permission it
can never hold (a hard-restricted one such as SMS) aborts unless
ALLOW_PARTIAL_PERMISSIONS=1accepts the weaker guarantee; and the results CSV and its log are reserved with exclusive
creates before the first device command, so two runs started against two devices in the same
second cannot interleave or truncate each other's evidence. Each of these has a named override
or a clear message naming the file or setting involved.
AIRPLANEis a radio-state control, not a connectivity one. It sets and verifies Wi-Fi andmobile data, in both directions. It deliberately does not claim association, DNS or endpoint
reachability, since nothing runnable at preflight establishes those for a whole run, so keeping the
external network condition stable is the operator's job, and the CSV records only what was
proved.
How to validate
Static analysis (from
tools/coldstart-benchmark/):End-to-end, against a device or emulator:
An A/A run must produce a mean block delta near zero with a CI straddling it, and no significant order effect.
Review checklist (to be filled by reviewers)