diff --git a/.github/workflows/integration-tests-macrobenchmark.yml b/.github/workflows/integration-tests-macrobenchmark.yml index 95a9ef4832..53dd462224 100644 --- a/.github/workflows/integration-tests-macrobenchmark.yml +++ b/.github/workflows/integration-tests-macrobenchmark.yml @@ -1,12 +1,20 @@ name: 'Integration Tests - Macrobenchmark' # Runs the sentry-uitest-android-macrobenchmark cold-start benchmark on a Sauce Labs real -# device and recovers timeToInitialDisplay from the device log. +# device and recovers its metrics from the device log. # +# The sample app is built twice -- once from the PR merged into its base, once from the base +# alone -- and both are installed on the device so the benchmark can alternate between them. +# Absolute numbers from a cloud device with unlocked CPU clocks are close to unreadable; a delta +# measured against a baseline on the same device in the same session is not. +# +# Runs on every pull request. One run costs two Gradle builds and up to an hour of a Sauce device +# at concurrency 1, so the concurrency group below matters: a new push supersedes the in-flight +# run for that PR rather than queueing behind it. on: - workflow_dispatch: + pull_request: concurrency: - group: ${{ github.workflow }}-${{ github.ref }} + group: ${{ github.workflow }}-${{ github.event.pull_request.number }} cancel-in-progress: true jobs: @@ -14,13 +22,36 @@ jobs: name: Macrobenchmark runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: write # to post the comparison back onto the PR + # we copy the secret to the env variable in order to access it in the workflow + # Note this is empty for pull requests from forks, which skips every step that needs Sauce. env: SAUCE_USERNAME: ${{ secrets.SAUCE_USERNAME }} steps: - name: Git checkout uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + # This checks out refs/pull/N/merge, the PR already merged into its base, so the + # candidate is what main will actually look like. Depth 2 is enough to resolve its + # first parent below. + fetch-depth: 2 + + # First parent of the merge commit is the base branch tip, which is therefore common to + # both arms: anything that landed on the base since the PR forked is present in each and + # cancels out, leaving the PR's own contribution. + - name: Resolve the base commit + id: base + run: echo "sha=$(git rev-parse HEAD^1)" >> "$GITHUB_OUTPUT" + + - name: Git checkout the base commit + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ steps.base.outputs.sha }} + path: baseline - name: 'Set up Java: 17' uses: actions/setup-java@b6effb05e454b25005698d916606bdc6ffcbf961 # v5 @@ -35,6 +66,34 @@ jobs: if: env.SAUCE_USERNAME != null run: ./gradlew :sentry-samples:sentry-samples-android:assembleRelease :sentry-android-integration-tests:sentry-uitest-android-macrobenchmark:assembleBenchmark + # Both builds share a Gradle user home, so this one starts with a warm cache. + - name: Assemble the baseline target app + if: env.SAUCE_USERNAME != null + working-directory: baseline + run: ./gradlew --build-cache :sentry-samples:sentry-samples-android:assembleRelease -PsampleAppIdSuffix=.baseline + + - name: Stage the baseline apk for Sauce + if: env.SAUCE_USERNAME != null + run: | + candidate=sentry-samples/sentry-samples-android/build/outputs/apk/release + base=baseline/$candidate + + # sampleAppIdSuffix has to exist in the *base* commit's build script for the baseline to + # get its own application id. A base predating that support silently builds a second copy + # of the same package, which Sauce would install over the candidate -- and the benchmark + # would then compare an app against itself and report a delta of roughly zero. Fail loudly + # instead. + base_id=$(jq -r .applicationId "$base/output-metadata.json") + candidate_id=$(jq -r .applicationId "$candidate/output-metadata.json") + if [ "$base_id" = "$candidate_id" ]; then + echo "::error::Baseline and candidate share the application id $base_id, so they cannot be installed side by side. The base commit ${{ steps.base.outputs.sha }} is missing the sampleAppIdSuffix property; rebase onto a base that has it." + exit 1 + fi + echo "baseline=$base_id candidate=$candidate_id" + + mkdir -p build/macrobenchmark-baseline + cp "$base/sentry-samples-android-release.apk" build/macrobenchmark-baseline/ + - name: Run Macrobenchmark in SauceLab uses: saucelabs/saucectl-run-action@283660aa934c02723c497efa151d582a3acc5801 # pin@v3 if: env.SAUCE_USERNAME != null @@ -46,15 +105,26 @@ jobs: config-file: .sauce/sentry-uitest-android-macrobenchmark.yml # Runs even when the suite fails: a failed benchmark still logs whatever it managed to - # measure, and the parser's own error explains what was missing. + # measure, and the parser's own error explains what was missing. summary.md is written only + # once the parser succeeds, so a failed recovery leaves no half-written report to post. - name: Recover benchmark results from the device log if: ${{ !cancelled() && env.SAUCE_USERNAME != null }} run: | - # Without pipefail the step passes on `tee`'s exit code, so a failed recovery - # would report success while silently producing no results. - set -o pipefail + mkdir -p ./artifacts python3 scripts/parse-macrobenchmark-log.py ./artifacts \ - --json-out ./artifacts/benchmarkData.json | tee -a "$GITHUB_STEP_SUMMARY" + --json-out ./artifacts/benchmarkData.json \ + --base-sha "${{ steps.base.outputs.sha }}" \ + --head-sha "${{ github.event.pull_request.head.sha }}" \ + > ./artifacts/summary.md.tmp + mv ./artifacts/summary.md.tmp ./artifacts/summary.md + cat ./artifacts/summary.md >> "$GITHUB_STEP_SUMMARY" + + - name: Comment the comparison on the PR + uses: marocchino/sticky-pull-request-comment@5770ad5eb8f42dd2c4f34da00c94c5381e49af88 # v3.0.5 + if: ${{ !cancelled() && env.SAUCE_USERNAME != null && hashFiles('./artifacts/summary.md') != '' }} + with: + header: macrobenchmark + path: ./artifacts/summary.md - name: Upload Sauce artifacts uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 diff --git a/.sauce/sentry-uitest-android-macrobenchmark.yml b/.sauce/sentry-uitest-android-macrobenchmark.yml index 4e3d5cfbb3..ac87357a59 100644 --- a/.sauce/sentry-uitest-android-macrobenchmark.yml +++ b/.sauce/sentry-uitest-android-macrobenchmark.yml @@ -11,18 +11,30 @@ sauce: - macrobenchmark defaults: - timeout: 40m + # 24 cold starts plus one full AOT compile per step, against 12 cold starts before the run + # started measuring two builds. + timeout: 60m espresso: app: ./sentry-samples/sentry-samples-android/build/outputs/apk/release/sentry-samples-android-release.apk testApp: ./sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/build/outputs/apk/benchmark/sentry-uitest-android-macrobenchmark-benchmark.apk + otherApps: + # The base-commit build of the same app, applicationId suffixed `.baseline` so it installs + # alongside the one above. The workflow stages it here; when it is missing, saucectl fails + # rather than silently measuring one build. + - ./build/macrobenchmark-baseline/sentry-samples-android-release.apk suites: - - name: "Macrobenchmark startup (api 35)" + - name: "Macrobenchmark startup A/B (api 35)" # No test orchestrator and no clearPackageData: Macrobenchmark manages its own process # restarts and AOT compilation, and StartupMode.COLD intentionally keeps app data and # permissions (it force-stops rather than `pm clear`). + appSettings: + # Sauce resigns the app under test on real devices, but never touches `otherApps`. Left on, + # the candidate would carry an injected agent the baseline does not and every delta would + # include the cost of that agent. + resigningEnabled: false devices: - id: Google_Pixel_9_Pro_XL_15_real_sjc1 # Google Pixel 9 Pro XL - api 35 (15) - high end diff --git a/scripts/parse-macrobenchmark-log.py b/scripts/parse-macrobenchmark-log.py index 280970dbea..c3fc987f48 100755 --- a/scripts/parse-macrobenchmark-log.py +++ b/scripts/parse-macrobenchmark-log.py @@ -1,23 +1,41 @@ #!/usr/bin/env python3 -"""Recover Macrobenchmark results from a Sauce Labs device log. +"""Recover Macrobenchmark results from a Sauce Labs device log and compare the two builds. -Sauce Labs cannot pull arbitrary files off a real device, so -SentryStartupBenchmark echoes its `-benchmarkData.json` into logcat as -numbered chunks. This reassembles those chunks and prints a Markdown summary. +Sauce Labs cannot pull arbitrary files off a real device, so SentryStartupBenchmark echoes its +`-benchmarkData.json` into logcat as numbered chunks. This reassembles those chunks and +prints a Markdown summary. + +When the run alternated between the base build and the build under test, the summary is a +base-vs-PR comparison. When only one build was installed -- a plain local run -- it falls back to +reporting that build on its own. Usage: parse-macrobenchmark-log.py [--json-out benchmarkData.json] + [--base-sha SHA] [--head-sha SHA] """ import argparse import json +import random import re +import statistics import sys from pathlib import Path # Must match SentryStartupBenchmark.LOG_TAG and its "[index/total]" chunk prefix. CHUNK_RE = re.compile(r"SentryBenchmarkData\s*:\s*\[(\d+)/(\d+)\](.*)$") +# Must match SentryStartupBenchmark.Variant. These appear in each benchmark's parameterized name, +# e.g. `startup[03-candidate]`. +BASELINE, CANDIDATE = "baseline", "candidate" + +# Enough resamples that the reported p is stable to about a thousandth, and still well under a +# second for the ~24 measurements a run produces. +RESAMPLES = 10000 + +# Fixed so re-running the parser on the same log always prints the same p. +SEED = 0 + def log_messages(log_file): """Yields the message text of every log entry. @@ -40,7 +58,12 @@ def log_messages(log_file): def collect_chunks(log_file): - """Returns one log's chunk texts keyed by index, plus the expected total.""" + """Returns one log's chunk texts keyed by index, plus the expected total. + + The benchmark may dump its results more than once (see logBenchmarkDataToLogcat). Later + chunks overwrite earlier ones and the file only ever grows, so what survives is the last, + most complete document. + """ chunks, total = {}, None for message in log_messages(log_file): match = CHUNK_RE.search(message) @@ -57,56 +80,190 @@ def reassemble(chunks, total): return "".join(chunks[i] for i in range(1, total + 1)) -def format_summary(data): +def variant_of(benchmark_name): + """Which build a benchmark entry measured, or None if the run wasn't labelled.""" + for variant in (BASELINE, CANDIDATE): + if variant in benchmark_name: + return variant + return None + + +def pool_runs(benchmarks): + """Returns {variant: {metric: [every iteration, across all steps]}}. + + Each step's own median and coefficientOfVariation are computed over a handful of iterations + and say nothing useful, so the per-iteration values are pooled per variant and the statistics + recomputed from those. + """ + pooled = {} + for benchmark in benchmarks: + variant = variant_of(benchmark["name"]) + for metric, result in benchmark["metrics"].items(): + pooled.setdefault(variant, {}).setdefault(metric, []).extend(result["runs"]) + return pooled + + +def summarize(runs): + return { + "min": min(runs), + "median": statistics.median(runs), + "max": max(runs), + "cov": statistics.stdev(runs) / statistics.fmean(runs) if len(runs) > 1 else 0.0, + "n": len(runs), + } + + +def permutation_p(base_runs, candidate_runs): + """Two-sided p-value for "the medians differ", by relabelling the measurements at random. + + Distribution-free, which suits a dozen noisy cold starts per arm better than a t-test: it + asks how often shuffling the same measurements between the two labels produces a median gap + at least as big as the one actually observed. A large p means the split we saw is an + unremarkable way to deal out these numbers -- not evidence of no change, just no evidence of + one. + """ + observed = abs(statistics.median(candidate_runs) - statistics.median(base_runs)) + pool = list(base_runs) + list(candidate_runs) + split = len(base_runs) + rng = random.Random(SEED) + at_least_as_extreme = 0 + for _ in range(RESAMPLES): + rng.shuffle(pool) + gap = abs(statistics.median(pool[split:]) - statistics.median(pool[:split])) + if gap >= observed: + at_least_as_extreme += 1 + # Add-one keeps p away from an unachievable 0. + return (at_least_as_extreme + 1) / (RESAMPLES + 1) + + +def format_header(data, base_sha, head_sha): context = data["context"] build = context["build"] lines = [ - "## Macrobenchmark results", - "", f"**Device:** {build['brand']} {build['model']} " f"(api {build['version']['sdk']}, {context['cpuCoreCount']} cores) · " f"**compilation:** {context['compilationMode']} · " f"**CPU clocks locked:** {context['cpuLocked']}", "", ] + if base_sha or head_sha: + lines[:0] = [ + f"**base:** `{base_sha or 'unknown'}` → **PR:** `{head_sha or 'unknown'}`", + "", + ] + return lines + + +def format_runs_details(label, runs): + return [ + "", + f"
{label} per iteration", + "", + ", ".join(f"{run:.1f}" for run in runs), + "", + "
", + ] + + +def format_comparison(data, pooled, base_sha, head_sha): + lines = ["## Macrobenchmark: PR vs base", ""] + lines += format_header(data, base_sha, head_sha) - if not context["cpuLocked"]: + if not data["context"]["cpuLocked"]: lines += [ - "> CPU clocks are unlocked on this device, so run-to-run spread is wide. " - "Treat these numbers as a trend, not a regression gate.", + "> CPU clocks are unlocked on this device, so the absolute numbers run high and wide. " + "The two builds were measured alternately on the same device in the same session, so " + "the delta is the trustworthy part -- not the medians either side of it.", "", ] - table = [ - "| Benchmark | Metric | min | median | max | CoV | iterations |", - "|---|---|--:|--:|--:|--:|--:|", + lines += [ + "| Metric | base median | PR median | Δ | Δ% | p | base min–max (CoV) | PR min–max (CoV) |", + "|---|--:|--:|--:|--:|--:|--:|--:|", ] details = [] - for benchmark in data["benchmarks"]: - name = f"{benchmark['className'].rsplit('.', 1)[-1]}.{benchmark['name']}" - for metric, result in sorted(benchmark["metrics"].items()): - table.append( - f"| `{name}` | {metric} " - f"| {result['minimum']:.1f} | {result['median']:.1f} | {result['maximum']:.1f} " - f"| {result['coefficientOfVariation'] * 100:.1f}% | {len(result['runs'])} |" - ) - runs = ", ".join(f"{run:.1f}" for run in result["runs"]) - details += [ - "", - f"
{metric} per iteration", - "", - runs, - "", - "
", - ] - - return "\n".join(lines + table + details) + for metric in sorted(set(pooled[BASELINE]) & set(pooled[CANDIDATE])): + # Only metrics both builds reported can be compared; the rest are called out below. + base_runs, candidate_runs = pooled[BASELINE][metric], pooled[CANDIDATE][metric] + base, candidate = summarize(base_runs), summarize(candidate_runs) + delta = candidate["median"] - base["median"] + delta_pct = delta / base["median"] * 100 if base["median"] else 0.0 + lines.append( + f"| {metric} | {base['median']:.1f} | {candidate['median']:.1f} " + f"| {delta:+.1f} | {delta_pct:+.1f}% " + f"| {permutation_p(base_runs, candidate_runs):.3f} " + f"| {base['min']:.1f}–{base['max']:.1f} ({base['cov'] * 100:.1f}%) " + f"| {candidate['min']:.1f}–{candidate['max']:.1f} ({candidate['cov'] * 100:.1f}%) |" + ) + details += format_runs_details(f"{metric} — base", base_runs) + details += format_runs_details(f"{metric} — PR", candidate_runs) + + iterations = len(next(iter(pooled[CANDIDATE].values()))) + lines += [ + "", + f"Δ is PR minus base, so negative is faster. {iterations} cold starts per build, " + "alternating between them so thermal drift lands on both. `p` is a permutation test on " + "the difference of medians: small means the gap is hard to explain as a reshuffle of the " + "same measurements. Reported for information only — this job never fails on it.", + ] + + one_sided = sorted(set(pooled[BASELINE]) ^ set(pooled[CANDIDATE])) + if one_sided: + lines += [ + "", + "> Only one build reported " + ", ".join(f"`{m}`" for m in one_sided) + ", so " + "there is nothing to compare it against and it is missing from the table above.", + ] + + if pooled.get(None): + lines += [ + "", + "> Some results carried no variant label and were left out of the table. That means " + "the benchmark ran a test this parser doesn't know about.", + ] + + return "\n".join(lines + details) + + +def format_single(data, pooled, base_sha, head_sha): + """Fallback for a run that measured one build, e.g. a local connectedBenchmarkAndroidTest.""" + variant, metrics = next(iter(pooled.items())) + lines = ["## Macrobenchmark results", ""] + lines += format_header(data, base_sha, head_sha) + lines += [ + f"> Only the **{variant or 'unlabelled'}** build was measured, so there is nothing to " + "compare against. Install both builds to get a base-vs-PR table.", + "", + "| Metric | min | median | max | CoV | iterations |", + "|---|--:|--:|--:|--:|--:|", + ] + details = [] + for metric in sorted(metrics): + runs = metrics[metric] + stats = summarize(runs) + lines.append( + f"| {metric} | {stats['min']:.1f} | {stats['median']:.1f} | {stats['max']:.1f} " + f"| {stats['cov'] * 100:.1f}% | {stats['n']} |" + ) + details += format_runs_details(metric, runs) + return "\n".join(lines + details) + + +def format_summary(data, base_sha=None, head_sha=None): + pooled = pool_runs(data["benchmarks"]) + if not pooled: + sys.exit("Benchmark data contains no results") + if BASELINE in pooled and CANDIDATE in pooled: + return format_comparison(data, pooled, base_sha, head_sha) + return format_single(data, pooled, base_sha, head_sha) def main(): parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("artifacts_dir", type=Path, help="directory of downloaded Sauce artifacts") parser.add_argument("--json-out", type=Path, help="where to write the recovered benchmarkData.json") + parser.add_argument("--base-sha", help="commit the baseline build came from") + parser.add_argument("--head-sha", help="commit the build under test came from") args = parser.parse_args() log_files = sorted(args.artifacts_dir.rglob("*.log")) @@ -135,7 +292,7 @@ def main(): if args.json_out: args.json_out.write_text(json.dumps(data, indent=2)) - print(format_summary(data)) + print(format_summary(data, args.base_sha, args.head_sha)) if __name__ == "__main__": diff --git a/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/README.md b/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/README.md index 9914825584..ae4cae65b2 100644 --- a/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/README.md +++ b/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/README.md @@ -1,7 +1,8 @@ # sentry-uitest-android-macrobenchmark Jetpack Macrobenchmark for cold-start of `sentry-samples-android`, used to evaluate SDK-init -performance changes on a real device in a **stable, reproducible** way. Not run in CI. +performance changes on a real device in a **stable, reproducible** way. Runs on Sauce Labs from the +`Integration Tests - Macrobenchmark` workflow, on every pull request. ## What it measures @@ -20,10 +21,45 @@ inspect the relevant slices directly (each iteration's trace is saved under `CompilationMode.Full()` pins ART AOT so dexopt state can't drift between runs. `StartupMode.COLD` does the correct force-stop sequencing (it does **not** `pm clear`, so app data/permissions are -kept). Iterations are capped at 12 because back-to-back cold starts thermally throttle an -unlocked-clock device after ~14 iterations, inflating the tail of longer runs. +kept). -## Running +## Comparing two builds + +Absolute cold-start numbers are only meaningful next to a baseline measured under the same +conditions, so the benchmark measures **two builds of the sample app at once**: + +| Variant | Package | Built from | +|---|---|---| +| `CANDIDATE` | `io.sentry.samples.android` | the PR merged into its base | +| `BASELINE` | `io.sentry.samples.android.baseline` | the base commit alone | + +Because the candidate is the *merge* of the PR into its base, anything that landed on the base +since the PR forked is present in both arms and cancels out, leaving the PR's own contribution. + +Both are installed on the same device and the run alternates between them, so whatever the device +does over the session — thermal throttling above all — lands on both and cancels out in the +difference. Two things follow from that, and both are deliberate: + +- **The run alternates ABBA, not ABAB.** In ABAB the candidate always follows the baseline, so + drift within a pair is charged to the candidate every single time. Mirroring each round makes + each variant the trailing one equally often. +- **The absolute numbers get worse, and that's fine.** 24 alternating cold starts throttle this + class of device where 12 back-to-back ones only start to. Read the delta, not the medians either + side of it. + +The suffixed applicationId comes from the `sampleAppIdSuffix` Gradle property, which +`sentry-samples-android` reads in its `defaultConfig`. The baseline build passes +`-PsampleAppIdSuffix=.baseline`; every ordinary build leaves it unset. + +That property has to exist in the **base** commit's build script, since that is what the baseline +is built from. Against a base predating it, both builds produce the same package and the workflow +fails with an explicit error rather than installing one over the other and comparing the app +against itself. + +If only the candidate is installed, the baseline steps are **skipped** (not failed) and the report +falls back to single-build numbers. + +## Running locally Connect a device, then: @@ -34,11 +70,55 @@ Connect a device, then: Results print to the console and are written to `build/outputs/connected_android_test_additional_output/.../*-benchmarkData.json`. +That measures the candidate only. To get the comparison locally, build a second copy of the app +with the suffix and install it alongside. Point the worktree at whatever you want as the baseline +— the merge base, or just `origin/main`: + +```bash +git worktree add ../baseline origin/main +(cd ../baseline && ./gradlew :sentry-samples:sentry-samples-android:assembleRelease \ + -PsampleAppIdSuffix=.baseline) +adb install -r ../baseline/sentry-samples/sentry-samples-android/build/outputs/apk/release/sentry-samples-android-release.apk +``` + +Then re-run the benchmark. `scripts/parse-macrobenchmark-log.py` also reads `adb logcat` output, so +the same base-vs-PR table can be produced from a local run. + +### Device hygiene (do this for trustworthy numbers) + +- **Wake and unlock the device first** — the launch check fails with "Unable to confirm activity + launch completion" on a dozing/locked screen + (`adb shell input keyevent KEYCODE_WAKEUP && adb shell wm dismiss-keyguard`). +- **Charge above 25%** — Macrobenchmark refuses to run below that. +- **Lock CPU clocks** if the device is rooted: this is the single biggest cure for thermal drift. +- Otherwise: let the device cool between runs, keep it on AC power, enable airplane mode, and turn + animations off (`adb shell settings put global window_animation_scale 0`, plus + `transition_animation_scale` and `animator_duration_scale`). +- Heed Macrobenchmark's warnings about unlocked clocks / low battery — they mean the numbers are + noisy. + ## Running on Sauce Labs -The `Integration Tests - Macrobenchmark` workflow (manual trigger) runs the same benchmark on a -Sauce Labs real device. It reports numbers only — it is not a PR gate, because cloud devices run -with unlocked CPU clocks and the run-to-run spread swamps most SDK-init changes. +The `Integration Tests - Macrobenchmark` workflow runs the same benchmark on a Sauce Labs real +device: it builds the sample app from the PR merge and from the base commit, ships both, and posts +the comparison to the job summary and to a PR comment it keeps updating. It reports numbers only +and never fails on them. + +It runs on every pull request. One run costs two Gradle builds and up to an hour of a Sauce device +at concurrency 1, so the workflow's concurrency group cancels a PR's in-flight run when a new push +arrives. If that turns out to be too expensive in aggregate, the cheapest lever is a `paths` filter +or a label gate on the job — neither changes anything else here. + +PRs from forks get no secrets, so the Sauce steps skip. Do not reach for `pull_request_target` to +change that: it would run a fork's build scripts with our Sauce credentials in scope. + +Two configuration details in `.sauce/sentry-uitest-android-macrobenchmark.yml` are load-bearing: + +- **`otherApps`** carries the baseline APK. Sauce installs dependent apps without instrumenting or + modifying them, and allows up to seven. +- **`appSettings.resigningEnabled: false`.** Sauce resigns the app under test on real devices but + never touches `otherApps`. Left on, the candidate would carry an injected agent the baseline does + not, and every delta would include the cost of that agent. Getting the numbers *back off* the device is the awkward part, so if you are changing this, know what has already been ruled out: @@ -58,28 +138,17 @@ what has already been ruled out: So `SentryStartupBenchmark` echoes its own `benchmarkData.json` into logcat in chunks, which reaches CI inside `device.log`, and `scripts/parse-macrobenchmark-log.py` reassembles it and -writes a `timeToInitialDisplay` table to the job summary. Note this recovers the metrics only — -the perfetto traces are megabytes each and cannot go through logcat, so sub-millisecond work -still needs a local device. - -### Device hygiene (do this for trustworthy numbers) - -- **Wake and unlock the device first** — the launch check fails with "Unable to confirm activity - launch completion" on a dozing/locked screen - (`adb shell input keyevent KEYCODE_WAKEUP && adb shell wm dismiss-keyguard`). -- **Charge above 25%** — Macrobenchmark refuses to run below that. -- **Lock CPU clocks** if the device is rooted: this is the single biggest cure for thermal drift. -- Otherwise: let the device cool between runs, keep it on AC power, enable airplane mode, and turn - animations off (`adb shell settings put global window_animation_scale 0`, plus - `transition_animation_scale` and `animator_duration_scale`). -- Heed Macrobenchmark's warnings about unlocked clocks / low battery — they mean the numbers are - noisy. - -## A/B-ing an SDK change - -Macrobenchmark measures one build per run, so compare separate runs — but **interleave them**: -running all of variant A followed by all of variant B lets thermal drift systematically penalize -whichever variant runs second. Instead, alternate A/B rounds (build variant A, run, build variant -B, run, repeat 2–3 times), keep each round's `*-benchmarkData.json`, and compare the values pooled -per variant. Prefer the `SentryAndroid.init` metric for SDK-init changes — it isolates init cost, so -it moves on changes that `timeToInitialDisplay` would bury in cold-start noise. +writes the comparison table to the job summary. Note this recovers the metrics only — the perfetto +traces are megabytes each and cannot go through logcat, so sub-millisecond work still needs a +local device. + +## Known limitations + +- **A base commit older than the `SentryAndroid.init` trace section** (added in #5901) has no such + section for `TraceSectionMetric` to find, and Macrobenchmark fails the whole run rather than + reporting the other metric. Rebase onto a newer `main` if you hit this. +- Both builds carry the same DSN, so baseline launches also send events to the sample project. + Harmless for the measurement. +- `p` in the report is a permutation test on the difference of medians. It says how hard the + observed gap is to explain as a reshuffle of the same measurements; a large `p` is not evidence + that nothing changed. diff --git a/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/src/main/AndroidManifest.xml b/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/src/main/AndroidManifest.xml index b2d3ea1235..a436d12f44 100644 --- a/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/src/main/AndroidManifest.xml +++ b/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/src/main/AndroidManifest.xml @@ -1,2 +1,14 @@ - + + + + + + + + + diff --git a/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/src/main/java/io/sentry/uitest/android/macrobenchmark/SentryStartupBenchmark.kt b/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/src/main/java/io/sentry/uitest/android/macrobenchmark/SentryStartupBenchmark.kt index 824a2a0628..bf5f243c1b 100644 --- a/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/src/main/java/io/sentry/uitest/android/macrobenchmark/SentryStartupBenchmark.kt +++ b/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/src/main/java/io/sentry/uitest/android/macrobenchmark/SentryStartupBenchmark.kt @@ -1,5 +1,6 @@ package io.sentry.uitest.android.macrobenchmark +import android.content.pm.PackageManager import android.util.Log import androidx.benchmark.macro.CompilationMode import androidx.benchmark.macro.ExperimentalMetricApi @@ -7,13 +8,32 @@ import androidx.benchmark.macro.StartupMode import androidx.benchmark.macro.StartupTimingMetric import androidx.benchmark.macro.TraceSectionMetric import androidx.benchmark.macro.junit4.MacrobenchmarkRule -import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry import java.io.File +import java.util.Locale import org.junit.AfterClass +import org.junit.Assume.assumeTrue import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith +import org.junit.runners.Parameterized + +/** The two builds of the sample app a run can measure. */ +enum class Variant(val packageName: String) { + /** The base-commit build, suffixed via the `sampleAppIdSuffix` property so both can coexist. */ + BASELINE("io.sentry.samples.android.baseline"), + + /** The build under test. The only variant present on a plain local run. */ + CANDIDATE("io.sentry.samples.android"), +} + +/** One `measureRepeated` call: which build to launch, and where it sits in the run order. */ +data class Step(private val index: Int, val variant: Variant) { + // Becomes the JUnit parameter name, so results arrive as `startup[01-baseline]`. The index keeps + // the run order visible in the raw data; the label is what parse-macrobenchmark-log.py groups on. + override fun toString() = + String.format(Locale.ROOT, "%02d-%s", index, variant.name.lowercase(Locale.ROOT)) +} /** * Cold-start benchmark for the sentry-samples-android app, used to evaluate SDK-init changes on a @@ -26,34 +46,79 @@ import org.junit.runner.RunWith * - SentryAndroid.init ([TraceSectionMetric]) — the duration of the `SentryAndroid.init` * [android.os.Trace] section the SDK emits, isolating SDK-init cost from the rest of the start. * + * When both builds are installed, the run alternates between them ([steps]) so that whatever the + * device does over the session — thermal throttling above all — lands on both and cancels out in + * the difference. Absolute numbers from an alternating run are therefore worse than from a short + * single-build run; the *delta* is what this is for. When only [Variant.CANDIDATE] is installed, + * the baseline steps are skipped and the run degrades to measuring one build. + * * [CompilationMode.Full] pins ART AOT compilation so dexopt state does not drift between runs. */ @OptIn(ExperimentalMetricApi::class) -@RunWith(AndroidJUnit4::class) -class SentryStartupBenchmark { +@RunWith(Parameterized::class) +class SentryStartupBenchmark(private val step: Step) { @get:Rule val benchmarkRule = MacrobenchmarkRule() @Test - fun startupFullCompilation() = + fun startup() { + // A local run normally has only the candidate installed. Skip rather than fail, so the run + // still produces single-build numbers. + assumeTrue( + "${step.variant.packageName} is not installed", + isInstalled(step.variant.packageName), + ) benchmarkRule.measureRepeated( - packageName = TARGET_PACKAGE, + packageName = step.variant.packageName, metrics = listOf(StartupTimingMetric(), TraceSectionMetric(INIT_TRACE_SECTION)), compilationMode = CompilationMode.Full(), startupMode = StartupMode.COLD, - iterations = 12, + iterations = ITERATIONS_PER_STEP, setupBlock = { pressHome() }, ) { startActivityAndWait() } + } - // Not private: @AfterClass needs a public static method. - companion object { - private const val TARGET_PACKAGE = "io.sentry.samples.android" + private fun isInstalled(packageName: String): Boolean = + try { + // Needs the entries in the module's AndroidManifest to see past package visibility. + InstrumentationRegistry.getInstrumentation() + .context + .packageManager + .getPackageInfo(packageName, 0) + true + } catch (e: PackageManager.NameNotFoundException) { + false + } + // Not private: @Parameters and @AfterClass need public static methods. + companion object { // Matches the android.os.Trace section name in SentryAndroid.init. private const val INIT_TRACE_SECTION = "SentryAndroid.init" + /** + * Cold starts per step. Kept small so the run alternates often, but not 1: every step pays for + * its own `CompilationMode.Full` AOT compile of the target. + */ + private const val ITERATIONS_PER_STEP = 3 + + /** How many times [AB_ROUND] repeats. 2 rounds x 3 iterations = 12 cold starts per variant. */ + private const val ROUNDS = 2 + + /** + * ABBA rather than ABAB: in ABAB the candidate always follows the baseline, so any drift within + * a pair is charged to the candidate every single time. Mirroring the second half of each round + * makes each variant the trailing one equally often. + */ + private val AB_ROUND = + listOf(Variant.BASELINE, Variant.CANDIDATE, Variant.CANDIDATE, Variant.BASELINE) + + @JvmStatic + @Parameterized.Parameters(name = "{0}") + fun steps(): List = + List(ROUNDS) { AB_ROUND }.flatten().mapIndexed { index, variant -> Step(index + 1, variant) } + private const val BENCHMARK_DATA_SUFFIX = "-benchmarkData.json" /** Kept in sync with `scripts/parse-macrobenchmark-log.py`. */ @@ -73,6 +138,10 @@ class SentryStartupBenchmark { * Safe to run at this point because `ResultWriter` writes the file synchronously as each result * is appended; only its *reporting* is deferred to the end of the run. Locally this is just * extra logcat noise — Gradle still copies the real file into the build directory. + * + * [Parameterized] runs this once, after the last step, so one dump covers every step. Were that + * ever to change, the parser would still cope: the file only grows, and its later-chunk-wins + * reassembly ends up with the most complete document in the log. */ @JvmStatic @AfterClass diff --git a/sentry-samples/sentry-samples-android/build.gradle.kts b/sentry-samples/sentry-samples-android/build.gradle.kts index 31009f6dbb..7e654ac9c3 100644 --- a/sentry-samples/sentry-samples-android/build.gradle.kts +++ b/sentry-samples/sentry-samples-android/build.gradle.kts @@ -47,6 +47,9 @@ android { defaultConfig { applicationId = "io.sentry.samples.android" + // Lets the macrobenchmark workflow build a second copy of this app that installs alongside the + // normal one, so it can cold-start both in a single session. Unset for every ordinary build. + applicationIdSuffix = providers.gradleProperty("sampleAppIdSuffix").orNull // androidx.sqlite 2.6+ require minSdk 23; the Sentry SDK still supports 21. minSdk = 23 targetSdk = libs.versions.targetSdk.get().toInt()