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())