From 66183a8596a6868bd0b5e6fc7ff4b91bb56c2810 Mon Sep 17 00:00:00 2001 From: Hadrian Burkhardt Date: Sat, 17 Oct 2026 23:00:22 +0200 Subject: [PATCH] The benchmark runs in CI, and fails when the jank metric is missing Item 5 of the queue: .github/workflows/benchmark.yml measures on an API 35 emulator (reactivecircus/android-emulator-runner, google_apis, x86_64, SwiftShader), on demand and every Monday. Not on pull requests: a run is minutes of measurement behind twenty of booting an emulator, and the gate is meant to answer in one or two. It installs both APKs and instruments directly instead of using connectedBenchmarkAndroidTest, because that task installs the app itself - which undoes the compile reset that makes the measurement a never-run baseline. tools/benchmark-report.py turns the JSON into two tables, keeping frameCount (a length) apart from frameDurationCpuMs and frameOverrunMs (speeds), and --require-frame-duration fails the job when the second table would be empty: the metric is the reason the job exists. Driving the workflow's own script against the phone first paid for itself three times. It found that cmd package compile --reset needs adbd as root - a user build refuses it with SecurityException, so the workflow now does adb root first (google_apis allows it) instead of silently measuring a compiled app. It produced the second run of the 20 000-file open (16 frames median, 32 max) and the first numbers for scroll, search and cold start at that size: search costs 319 frames, which is now the biggest number in the file. And it showed the phone reporting frameDurationCpuMs and frameOverrunMs after all, so PERFORMANCE.md's 'this phone reports counts only' was a property of those earlier runs, not of the handset - corrected, with the numbers. --- .github/workflows/benchmark.yml | 111 ++++++++++++++++++++++ .gitignore | 7 ++ docs/PERFORMANCE.md | 83 +++++++++++++--- docs/ROADMAP.md | 10 +- tools/benchmark-report.py | 163 ++++++++++++++++++++++++++++++++ 5 files changed, 357 insertions(+), 17 deletions(-) create mode 100644 .github/workflows/benchmark.yml create mode 100755 tools/benchmark-report.py diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml new file mode 100644 index 0000000..8526c21 --- /dev/null +++ b/.github/workflows/benchmark.yml @@ -0,0 +1,111 @@ +name: benchmark + +# The macrobenchmark run, on an emulator rather than on a phone. +# +# Three reasons this exists as a workflow and not as a local step: +# +# * **It reports the jank metric.** `frameDurationCpuMs` and `frameOverrunMs` need frame timelines, which +# this project's phone sometimes supplies and sometimes does not — the run that does is the one worth +# looking at, and a CI emulator supplies it every time. +# * **It takes the phone out of the loop.** No MIUI install prompts, no AVD to keep alive, no cable. +# * **It is a second opinion.** The numbers in docs/PERFORMANCE.md come from one handset; an emulator run +# is a different device and build, so a regression that is the app's shows up in both. +# +# It is not on every pull request: one run is the four benchmarks × five iterations, which is minutes of +# measurement and considerably more of waiting for an emulator to boot. +on: + workflow_dispatch: + schedule: + # Monday, early: the week starts with a number, not with a guess. + - cron: '41 4 * * 1' + +concurrency: + group: benchmark + cancel-in-progress: false + +jobs: + emulator: + runs-on: ubuntu-latest + timeout-minutes: 75 + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-java@v4 + with: + distribution: temurin + # detekt 1.23 cannot parse JDK 25's version string, and the gate runs on 21 — keep them equal + # so a benchmark run cannot fail for a reason unrelated to the benchmark. + java-version: '21' + - uses: android-actions/setup-android@v3 + - uses: gradle/actions/setup-gradle@v4 + + # Built before the emulator starts: Gradle is quick on the runner, emulator boot is not, and the + # two would otherwise queue behind each other. + - name: Build the APKs + run: ./gradlew :app:assembleBenchmark :benchmark:assembleBenchmark --console=plain + + - name: Measure on an emulator + uses: reactivecircus/android-emulator-runner@v2 + with: + api-level: 35 + # google_apis, not google_apis_playstore: the benchmark toggles the app's all-files access in + # Settings, and a Play image has no such page for a sideloaded app. + target: google_apis + arch: x86_64 + profile: pixel_6 + disable-animations: true + emulator-boot-timeout: 1200 + # SwiftShader because the runner has no GPU: software rendering makes the milliseconds + # incomparable with a phone's, which is stated where the numbers are read. What survives is the + # shape — which benchmark is slow relative to the others — and the existence of the metric. + emulator-options: >- + -no-snapshot -no-window -gpu swiftshader_indirect -noaudio -no-boot-anim + -camera-back none -camera-front none + script: | + set -o pipefail + # The fixture is built through adb because UiAutomation's shell runs as the app, not the shell + # user, and cannot write to /sdcard (see tools/benchmark-fixture.sh). + ./tools/benchmark-fixture.sh + adb install -r app/build/outputs/apk/benchmark/app-benchmark.apk + adb install -r benchmark/build/outputs/apk/benchmark/benchmark-benchmark.apk + # The "never run" baseline: no profile, so what is measured has not been compiled by use. + # Clearing profile data is a system operation, so it needs adbd as root — which a google_apis + # image allows and a user build (the phone) does not, where this silently stays best-effort. + adb root || true + adb wait-for-device + adb shell cmd package compile --reset de.softwareapp_hb.filemanager || true + # Instrumented directly rather than through connectedBenchmarkAndroidTest, because that task + # installs the app itself — which would undo the compile reset above. `CompilationMode.Ignore()` + # in the test means nothing else installs either. + adb shell am instrument -w -r \ + -e class de.softwareapp_hb.filemanager.benchmark.BrowserBenchmark \ + de.softwareapp_hb.filemanager.benchmark/androidx.test.runner.AndroidJUnitRunner \ + | tee instrument.log + status=${PIPESTATUS[0]} + mkdir -p benchmark-results + adb pull /sdcard/Android/media/de.softwareapp_hb.filemanager.benchmark benchmark-results/ || true + # The jank metric is the reason this job exists, so its absence fails the run rather than + # becoming a footnote in a log nobody opens. + python3 tools/benchmark-report.py --require-frame-duration benchmark-results > summary.md || status=1 + cat summary.md + exit $status + + - name: Keep the numbers + if: always() + uses: actions/upload-artifact@v4 + with: + name: benchmark-results + path: | + benchmark-results + instrument.log + summary.md + if-no-files-found: warn + + - name: Put the table on the run page + if: always() + run: | + if [ -f summary.md ]; then + cat summary.md >> "$GITHUB_STEP_SUMMARY" + else + echo "No summary: the benchmark did not get far enough to write one. See instrument.log." \ + >> "$GITHUB_STEP_SUMMARY" + fi diff --git a/.gitignore b/.gitignore index 604b9c4..39677dc 100644 --- a/.gitignore +++ b/.gitignore @@ -32,3 +32,10 @@ demo-files/ # Written by :app:generateReleaseBaselineProfile; the profile itself is committed at # app/src/main/baseline-prof.txt. app/src/release/generated/ + +# Written by a benchmark run, locally or by .github/workflows/benchmark.yml: the pulled JSON and traces, +# the instrumentation log and the table built from them. They describe one device at one moment; the +# numbers that matter are copied into docs/PERFORMANCE.md. +benchmark-results/ +instrument.log +summary.md diff --git a/docs/PERFORMANCE.md b/docs/PERFORMANCE.md index cf80910..5e7b4fa 100644 --- a/docs/PERFORMANCE.md +++ b/docs/PERFORMANCE.md @@ -117,11 +117,19 @@ change in the app would show up as a change in these numbers rather than as nois **Those frame counts are not a jank metric, and the difference matters.** `FrameTimingMetric` normally publishes `frameDurationCpuMs` and `frameOverrunMs` — how long each frame took and how far past its -deadline it ran. This phone reports neither: the same run's JSON contains `frameCount` and nothing else, -so "14 frames to open the folder" is how many frames the interaction took, not how long any of them -lasted. It says the interaction is short (14 frames is about a quarter of a second at 60 Hz); it cannot -say whether any of those frames was slow. A device that reports frame durations — an emulator or any -non-MIUI phone — is what turns this into a jank number. +deadline it ran. `frameCount`, which is what the table above is made of, is how many frames an interaction +took: a length, not a speed. It says a folder opens in about a fifth of a second at 60 Hz; it cannot say +whether any of those frames was slow. + +The runs those numbers come from reported `frameCount` only — their JSON had no sampled metrics at all, +which is what started the search for a device that supplies them. **A later run on the same phone did +supply them**, so the absence was a property of those runs, not of the handset. For the 1 000-file open, +measured after the listing was made progressive, `frameDurationCpuMs` is **P50 13.9 ms, P90 26.5 ms, +P99 86.5 ms** and `frameOverrunMs` **P50 19.0 ms, P90 32.2 ms, P99 80.6 ms**, next to `frameCount` median +12 (max 48) from the same run. Against a 60 Hz budget of 16.7 ms that is "the median frame fits, one in ten +takes 26 ms" — a statement the counts could never make. Why the earlier runs captured no frame timelines +is not known; what is known is that a run either does or does not, and that `.github/workflows/benchmark.yml` +now runs it on an emulator where it always does. ### How it scales with folder size @@ -204,6 +212,31 @@ batches still take the ~2 s described above — they arrive behind the progress already scrolling rather than into an empty screen. Both halves matter, and only the first one is in this table. +### The whole suite at 20 000 files, twice + +The script that the CI workflow runs was also run against the same 20 000-file fixture — it is +device-agnostic apart from the emulator runner — which gave a second, independent run of the open +benchmark and, for the first time, the other three at that size: + +| 20 000 files | one value per iteration | min–max | `frameDurationCpuMs` P50 / P90 / P99 | +|---|---|---|---| +| `openAFolderOfAThousandFiles` | **16 frames** | 13–32 | 7.8 / 21.6 / 100.6 ms | +| `scrollAFolderOfAThousandFiles` | **162 frames** | 148–172 | 9.0 / 21.0 / 25.2 ms | +| `searchInsideAFolderOfAThousandFiles` | **319 frames** | 300–653 | 6.5 / 11.5 / 18.5 ms | +| `coldStart` | **599 ms** to first display | 593–604 | — | + +Two things in it are worth keeping: + +* **The batching result holds in a second run.** The table above has 12 median / 48 max from one run; this + one has 16 median / 32 max on a build that also carries the M16 picker. That is the same number within + the noise, and the 598-frame stall has not come back. +* **Search is the next outlier.** 319 frames is about five seconds from the query to `file-99.txt`, + against 38–39 frames at 1 000 files: twenty times the entries for eight times the frames, so sublinear — + but still long enough to notice, and the per-frame numbers say drawing is not the reason (P90 11.5 ms). + Handing twenty thousand rows to a list in one go is; that is where a "large folder" pass would start. + `coldStart`'s 599 ms and 162 frames of scrolling are recorded here as the baseline those two get + compared against, not as findings. + And the fixture now survives: `tools/benchmark-fixture.sh --count 20000` reuses a folder that already holds at least that many files, and `--fresh` rebuilds it. Building it takes about half an hour through MIUI's storage layer, which is the whole reason to keep it. @@ -317,17 +350,37 @@ instrumentation output itself, and the per-test JSON and Perfetto traces are lef `/sdcard/Android/media/de.softwareapp_hb.filemanager.benchmark/` — they used to be deleted with the test app when a run ended, which is why the earlier numbers had to be copied mid-run. +### The same thing in CI + +`.github/workflows/benchmark.yml` runs those four commands on an emulator +(`reactivecircus/android-emulator-runner`, API 35, `google_apis`, x86_64, SwiftShader), on demand and every +Monday morning. It is deliberately not part of the pull-request gate: a run is minutes of measurement behind +twenty minutes of booting an emulator, and the gate is meant to answer in one or two minutes. + +Two details make the numbers meaningful rather than merely present: + +* The app is installed and `cmd package compile --reset` **before** the instrumentation starts, and the run + is driven with `am instrument` rather than `connectedBenchmarkAndroidTest` — the Gradle task installs the + app itself, which would undo that reset and quietly measure a compiled app. +* `tools/benchmark-report.py --require-frame-duration` turns the results into a table on the run's page and + **fails the job** when `frameDurationCpuMs` is missing. That metric is the reason the job exists; a run + without it is not a green run with a footnote. + +The emulator's milliseconds are not a phone's — software rendering, a different CPU, no thermal limits — so +they are read as shape and trend (which benchmark is slow relative to the others, whether a change moved it), +not as an absolute. + ## Still not measured | number | status | |---|---| -| Folder open with 5 000 / 20 000 files | not measured yet: `./tools/benchmark-fixture.sh --count 20000` builds that fixture, and the open/scroll numbers follow from re-running the same command | +| Folder open with 5 000 / 20 000 files | **measured**: see the scaling table above (`--count N` builds the fixture) | | Thumbnail paint while scrolling a photo grid | not measured: no test for it yet | -| Any jank figure at all | not measurable here: this phone's `FrameTimingMetric` output is `frameCount` only, with no `frameDurationCpuMs` or `frameOverrunMs` | +| Any jank figure at all | **measured**: `frameDurationCpuMs` and `frameOverrunMs` both appear in the phone's 1 000-file run above, and the CI emulator run reports them every time | -What the interaction numbers still need is a device that reports frame durations — an emulator or any -non-MIUI phone would do, and the fixture is a folder of empty files, so nothing device-specific is being -measured. The install side is solved: `CompilationMode.Ignore()` plus installing from the host means a run +What is left for the interaction numbers is a device class decision rather than a measurement: the phone +numbers are one handset's, the emulator's are software-rendered, and neither is a claim about a mid-range +device. The install side is solved: `CompilationMode.Ignore()` plus installing from the host means a run touches the package manager zero times. ## What is measured instead, meanwhile @@ -372,8 +425,8 @@ adb shell cmd package compile --reset de.softwareapp_hb.filemanager # n adb shell cmd package compile -m speed-profile -f de.softwareapp_hb.filemanager # profile applied ``` -The emulator did answer one open question, though: on that path `FrameTimingMetric` **does** report what -the phone cannot — `frameDurationCpuMs` and `frameOverrunMs` are both present (P50 233 ms and 341 ms for -opening the folder, which is software rendering and not a number anyone should quote). The metric exists -as soon as the device supports frame timelines, so a physical API 33+ phone gives both the profile -delta and the jank figures the interaction benchmarks have been missing. +The emulator did answer one open question, though: on that path `FrameTimingMetric` reports both +`frameDurationCpuMs` and `frameOverrunMs` (P50 233 ms and 341 ms for opening the folder, which is software +rendering and not a number anyone should quote). The metric exists as soon as the device captures frame +timelines — which is what `.github/workflows/benchmark.yml` now guarantees, and what a physical API 33+ +phone would combine with the profile delta. diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 9767d55..7218d32 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -89,8 +89,14 @@ release should not go out without. the selection rule. Driven on the phone for both verbs (see `EDGE_CASE_TESTING.md`), which is what caught the picker naming internal storage by its raw name `0`. **Not done**: the "copied to X — Show" message with a jump to the destination; the copy reports itself the way a paste does. -5. **A CI run of the benchmarks on an emulator** — removes the phone from the loop and reports - `frameDurationCpuMs`, which this device cannot. +5. **A CI run of the benchmarks on an emulator** — **done**: `.github/workflows/benchmark.yml` runs the four + benchmarks on API 35 (`reactivecircus/android-emulator-runner`, `google_apis`, x86_64, SwiftShader) on + demand and every Monday, installs the app and resets its compilation before instrumenting it, pulls the + JSON, and publishes `tools/benchmark-report.py`'s table on the run page — failing the job if + `frameDurationCpuMs` is missing, since that metric is why the job exists. While wiring it up, the + phone's own JSON from the progressive-listing run turned out to contain `frameDurationCpuMs` and + `frameOverrunMs` after all (P50 13.9 ms / 19.0 ms for the 1 000-file open), so `PERFORMANCE.md`'s "counts + only" finding was a property of the earlier runs, not of the device; corrected there. ### M16 — Copy to… / Move to… diff --git a/tools/benchmark-report.py b/tools/benchmark-report.py new file mode 100755 index 0000000..b1ed13e --- /dev/null +++ b/tools/benchmark-report.py @@ -0,0 +1,163 @@ +#!/usr/bin/env python3 +"""Turn macrobenchmark's JSON into a table a human reads, and say when the jank metric is missing. + + ./tools/benchmark-report.py benchmark-results # after `adb pull` + ./tools/benchmark-report.py --require-frame-duration results/ + +Macrobenchmark writes one `*-benchmarkData.json` per run into the *test* app's media directory: + + /sdcard/Android/media/de.softwareapp_hb.filemanager.benchmark/ + +and it has two kinds of number in it, which mean different things and are therefore kept apart: + +* `metrics` — one value per iteration. `frameCount` is how many frames an interaction took: a length, + not a speed. +* `sampledMetrics` — many values per iteration, summarised as percentiles. `frameDurationCpuMs` (how + long each frame took) and `frameOverrunMs` (how far past its deadline it ran) are the jank numbers. + +This phone reports `frameCount` in the first group and, on some runs, `frameDurationCpuMs` in the +second; whether the second group has the jank metric is exactly what a CI emulator run is for, so +`--require-frame-duration` makes its absence a failure rather than a footnote. +""" + +from __future__ import annotations + +import argparse +import glob +import json +import os +import sys + + +def files_in(paths: list[str]) -> list[str]: + """Every `*-benchmarkData.json` under the given files and directories, in a stable order.""" + found: list[str] = [] + for path in paths: + if os.path.isdir(path): + found += glob.glob(os.path.join(path, "**", "*.json"), recursive=True) + else: + found.append(path) + return sorted(set(found)) + + +def load(path: str) -> dict | None: + """The run in [path], or `None` when the file is not macrobenchmark output.""" + try: + with open(path, encoding="utf-8") as handle: + data = json.load(handle) + except (OSError, json.JSONDecodeError): + return None + return data if isinstance(data, dict) and "benchmarks" in data else None + + +def device_line(context: dict) -> str: + build = context.get("build", {}) + sdk = build.get("version", {}).get("sdk") + name = " ".join(part for part in (build.get("brand"), build.get("model")) if part) + fingerprint = build.get("fingerprint", "") + # The fingerprint is the honest identifier: same model, different build, different numbers. + return f"{name} (API {sdk}) — `{fingerprint}`" if fingerprint else f"{name} (API {sdk})" + + +def number(value: float) -> str: + """A value with as much precision as it needs: counts are whole, milliseconds are not.""" + return str(int(value)) if float(value).is_integer() else f"{value:.1f}" + + +def report(data: dict) -> tuple[list[str], bool]: + """Markdown lines for one run, and whether it carried per-frame timings.""" + lines: list[str] = [] + context = data.get("context", {}) + lines.append(f"**Device**: {device_line(context)}") + lines.append(f"**Compilation**: `{context.get('compilationMode', 'unknown')}`") + lines.append("") + + benchmarks = data.get("benchmarks", []) + has_frame_duration = any("frameDurationCpuMs" in b.get("sampledMetrics", {}) for b in benchmarks) + + counted = sorted((b for b in benchmarks if b.get("metrics")), key=lambda b: b["name"]) + if counted: + # One value per iteration: `frameCount` is a length in frames, `timeToInitialDisplayMs` a + # duration. They share a table because they are read the same way, but the unit is per row. + lines.append("### One value per iteration") + lines.append("") + lines.append("| benchmark | metric | median | min | max | iterations |") + lines.append("|---|---|---|---|---|---|") + for benchmark in counted: + for metric, stats in sorted(benchmark["metrics"].items()): + runs = len(stats.get("runs", [])) + unit = "frames" if metric == "frameCount" else "ms" + lines.append( + f"| `{benchmark['name']}` | {metric} | {number(stats['median'])} {unit} | " + f"{number(stats['minimum'])} | {number(stats['maximum'])} | {runs} |", + ) + lines.append("") + + sampled = sorted((b for b in benchmarks if b.get("sampledMetrics")), key=lambda b: b["name"]) + if sampled: + lines.append("### Per-frame timing") + lines.append("") + lines.append("| benchmark | metric | P50 | P90 | P99 |") + lines.append("|---|---|---|---|---|") + for benchmark in sampled: + for metric, stats in sorted(benchmark["sampledMetrics"].items()): + lines.append( + f"| `{benchmark['name']}` | {metric} | {number(stats['P50'])} ms | " + f"{number(stats['P90'])} ms | {number(stats['P99'])} ms |", + ) + lines.append("") + + if not has_frame_duration: + lines.append("> No `frameDurationCpuMs` in this run: the device reported frame counts only, so " + "there is no jank number here — only how many frames each interaction took.") + lines.append("") + + total = sum(b.get("totalRunTimeNs", 0) for b in benchmarks) / 1e9 + plural = "benchmark" if len(benchmarks) == 1 else "benchmarks" + lines.append(f"_{len(benchmarks)} {plural}, {total:.0f} s of measurement._") + return lines, has_frame_duration + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument( + "paths", + nargs="*", + help="files or directories of macrobenchmark JSON (default: benchmark-results)", + ) + parser.add_argument( + "--require-frame-duration", + action="store_true", + help="exit non-zero unless the run reported frameDurationCpuMs", + ) + arguments = parser.parse_args() + paths = arguments.paths or ["benchmark-results"] + + runs = [(path, data) for path in files_in(paths) if (data := load(path)) is not None] + if not runs: + print(f"benchmark-report: no macrobenchmark JSON in {', '.join(paths)}", file=sys.stderr) + return 2 + + anywhere = False + for index, (path, data) in enumerate(runs): + if len(runs) > 1: + print(f"## {os.path.basename(path)}") + print() + lines, has_frame_duration = report(data) + anywhere = anywhere or has_frame_duration + print("\n".join(lines)) + if index < len(runs) - 1: + print() + + if arguments.require_frame_duration and not anywhere: + print( + "benchmark-report: frameDurationCpuMs is absent from every run, so no jank metric was " + "measured", + file=sys.stderr, + ) + return 1 + return 0 + + +if __name__ == "__main__": + sys.exit(main())