diff --git a/benchmark/src/main/AndroidManifest.xml b/benchmark/src/main/AndroidManifest.xml index 5b767b9..56fbfe6 100644 --- a/benchmark/src/main/AndroidManifest.xml +++ b/benchmark/src/main/AndroidManifest.xml @@ -1,3 +1,22 @@ - - + + + + + + + diff --git a/benchmark/src/main/java/de/softwareapp_hb/filemanager/benchmark/BrowserBenchmark.kt b/benchmark/src/main/java/de/softwareapp_hb/filemanager/benchmark/BrowserBenchmark.kt index 36b49f5..a1f8df2 100644 --- a/benchmark/src/main/java/de/softwareapp_hb/filemanager/benchmark/BrowserBenchmark.kt +++ b/benchmark/src/main/java/de/softwareapp_hb/filemanager/benchmark/BrowserBenchmark.kt @@ -7,11 +7,9 @@ import androidx.benchmark.macro.StartupMode import androidx.benchmark.macro.StartupTimingMetric import androidx.benchmark.macro.junit4.MacrobenchmarkRule import androidx.test.ext.junit.runners.AndroidJUnit4 -import androidx.test.platform.app.InstrumentationRegistry import androidx.test.uiautomator.By import androidx.test.uiautomator.UiDevice import androidx.test.uiautomator.Until -import org.junit.AfterClass import org.junit.Assert.assertNotNull import org.junit.Rule import org.junit.Test @@ -50,6 +48,12 @@ class BrowserBenchmark { */ private val app = "de.softwareapp_hb.filemanager.perf" + // No setup by shell command here on purpose. UiAutomation's executeShellCommand runs with *this + // app's* uid, not the shell user's, so it cannot create files in /sdcard and cannot grant itself + // an app-op — the first attempt failed exactly that way, with a RuntimeException from + // UiAutomationConnection and no fixture on disk. `tools/benchmark-fixture.sh` prepares the device + // instead: it runs through adb, so it has the shell's rights. + @Test fun coldStart() = benchmark.measureRepeated( packageName = app, @@ -119,11 +123,11 @@ class BrowserBenchmark { // ------------------------------------------------------------------ setup /** - * Everything the measured block must not pay for: the access grant, the fixture on disk, and the - * app open on the volume list with the fixture's row on screen. + * The device is expected to be prepared already: `tools/benchmark-fixture.sh` creates the fixture + * and grants the app its storage access. This only navigates to it, and fails with that + * instruction if the row is not where it should be. */ private fun MacrobenchmarkScope.openFixture(device: UiDevice) { - Fixture.ensure(device, app) pressHome() startActivityAndWait() val volume = device.wait(Until.findObject(By.text("Internal storage")), TIMEOUT_MS) @@ -134,7 +138,8 @@ class BrowserBenchmark { ) volume!!.click() assertNotNull( - "the fixture folder is not in the volume root, so setup did not complete", + "the fixture folder is not in the volume root. Run tools/benchmark-fixture.sh first — " + + "it creates /sdcard/${Fixture.NAME} on the device and grants the app storage access.", device.wait(Until.findObject(By.text(Fixture.NAME)), TIMEOUT_MS), ) } @@ -144,21 +149,12 @@ class BrowserBenchmark { /** The app bar's own description for the search action; see strings.xml. */ private const val SEARCH_ACTION = "Search this folder" - - /** - * Removes the fixture once the class is done. A benchmark that leaves a thousand files in - * someone's storage is a benchmark that stops being run — and this runs on real phones. - */ - @JvmStatic - @AfterClass - fun cleanUp() { - Fixture.remove(UiDevice.getInstance(InstrumentationRegistry.getInstrumentation())) - } } } /** - * The folder the interaction benchmarks use: a thousand empty files, created once and reused. + * The folder the interaction benchmarks use: a thousand empty files, created and removed by + * `tools/benchmark-fixture.sh` rather than by the test — see the note on the class. * * It sits at the root of shared storage under a name that sorts before everything else, because the * alternative — scrolling until the fixture appears — is another interaction to keep working as the @@ -167,30 +163,5 @@ class BrowserBenchmark { private object Fixture { const val NAME = "AABenchmark" const val FIRST_FILE = "file-0.txt" - private const val COUNT = 1_000 - private const val DIRECTORY = "/sdcard/$NAME" - private const val MARKER = "$DIRECTORY/.ready-$COUNT" - - fun ensure(device: UiDevice, app: String) { - // The benchmark build is not the debug build, so it starts without the access grant. Granting - // it through appops is what makes an unattended run possible: the alternative is a permission - // dialog no test can answer. - shell(device, "appops set --uid $app MANAGE_EXTERNAL_STORAGE allow") - - if (shell(device, "test -f $MARKER && echo ready || echo missing") == "ready") return - - shell(device, "rm -rf $DIRECTORY; mkdir -p $DIRECTORY") - // One shell loop rather than a thousand pushes: `: > file` is a builtin redirection, so this - // costs a single shell and no fork per file. - shell(device, "i=0; while [ \$i -lt $COUNT ]; do : > $DIRECTORY/file-\$i.txt; i=\$((i+1)); done") - shell(device, "touch $MARKER") - } - - fun remove(device: UiDevice) { - shell(device, "rm -rf $DIRECTORY") - } - - private fun shell(device: UiDevice, command: String): String = - device.executeShellCommand(command).trim() } diff --git a/docs/CODE_MAP.md b/docs/CODE_MAP.md index 39ba4d5..7e4fe53 100644 --- a/docs/CODE_MAP.md +++ b/docs/CODE_MAP.md @@ -2,7 +2,7 @@ Generated from the source by `tools/make-code-map.py` - do not edit by hand. Regenerate it after a change (`python3 tools/make-code-map.py`); `--check` exits non-zero when this file is stale. -77 source files, 12688 lines - 33 test files, 386 tests. The behaviour index at the end lists every test name, and in this project a test name is a sentence about what the app does, so it doubles as the specification. +77 source files, 12659 lines - 33 test files, 386 tests. The behaviour index at the end lists every test name, and in this project a test name is a sentence about what the app does, so it doubles as the specification. Start at `AGENTS.md`. What is done, and why: `docs/ROADMAP.md` and `CHANGELOG.md`. What was verified by hand rather than by a test: `docs/EDGE_CASE_TESTING.md`. @@ -170,7 +170,7 @@ The macrobenchmark test APK: four measurements over a fixture folder, run on a d | file | lines | what it holds | tests | |---|---|---|---| -| `BrowserBenchmark.kt` | 196 | The four numbers docs/PERFORMANCE.md promises, measured on a benchmark build of the app. BrowserBenchmark, Fixture | - | +| `BrowserBenchmark.kt` | 167 | The four numbers docs/PERFORMANCE.md promises, measured on a benchmark build of the app. BrowserBenchmark, Fixture | - | ## Behaviour index diff --git a/docs/PERFORMANCE.md b/docs/PERFORMANCE.md index a38a721..deb92dc 100644 --- a/docs/PERFORMANCE.md +++ b/docs/PERFORMANCE.md @@ -13,14 +13,23 @@ Until this file existed, the app had **no performance number of any kind**. | build | device | runs (ms) | median | |---|---|---|---| | debug, `1.0.0-debug` | Xiaomi phone, Android 15 | 1 758 · 1 773 · 1 814 · 1 926 · 2 043 | **1 814 ms** | +| `benchmark` (R8, not debuggable, profileable) | same | — | **532 ms** | -Taken with `./tools/measure-startup.sh 5`: `am start -W`, and one warm-up start discarded because the -first launch after an install pays for dexopt and folding that into a median is how a baseline gets -written that nothing can meet. +The first is `./tools/measure-startup.sh 5` (`am start -W`, one warm-up start discarded because the first +launch after an install pays for dexopt). The second is `BrowserBenchmark#coldStart` in the `:benchmark` +module: `StartupTimingMetric`, five iterations, `CompilationMode.None()`, measured as time to initial +display — min 499 ms, median 532 ms, max 805 ms. Measured twice: 615 ms before the storage grant was in +place (the app was showing the permission screen) and 532 ms after, which is the figure above. -The debug build is deliberately the *slow* case — StrictMode on, LeakCanary attached, no R8 — so this -is an upper bound rather than what a user sees. **The release number is the one worth publishing, and -it has not been taken yet.** Until it is, no speed claim belongs in the README or the store listing. +Two caveats belong to that number rather than to a footnote. The tool itself warns *"Unable to clear +Runtime Image, subsequent launches/iterations may exhibit faster startup than production due to +accelerated class loading"*, so it is optimistic by an unmeasured amount on this Android version. And +`CompilationMode.None()` means no compilation profile is applied — what a fresh install does — so a +baseline profile would lower it further and is the obvious next measurement. + +The debug build is deliberately the *slow* case — StrictMode on, LeakCanary attached, no R8 — which is +why the same screen takes it three times as long. That is the honest illustration of why a debug number +must never be published as the app's performance. ### What StrictMode found on the first cold start @@ -59,28 +68,31 @@ in `@AfterClass` — and the storage grant is applied with `appops`, so an unatt baseline. `CompilationMode.Partial()` plus a baseline profile is the follow-up, and needs the same module. -**They have not produced numbers yet.** On this phone `adb install` of the new package id is refused with -`INSTALL_FAILED_USER_RESTRICTED` — MIUI's "Install via USB" gate, which wants a human — so the two APKs -were pushed to `/sdcard/Download/` (`PlainFiles-perf.apk`, `PlainFiles-benchmark-test.apk`) to be -installed by hand. Once they are on the device: +**Two of these have produced a number** (`coldStart`, above); the three interaction ones have not, and +the reason is the device rather than the code. Getting them to run at all took four fixes, each of which +is written next to the code it belongs to: the app under test needs its own package id; it needs to be +installed by hand once, because MIUI refuses `adb install` of a new package id without a human +(`INSTALL_FAILED_USER_RESTRICTED`); the test APK needs package-visibility for the app it measures, or +macrobenchmark reports `NameNotFoundException` for an app that `pm list packages` shows installed; and +the fixture has to be created through adb, because `UiAutomation.executeShellCommand` runs with the +*test app's* uid and cannot write to `/sdcard`. + +The workflow that avoids all of it after the first install: ```bash +# once, by hand on the phone (Files → Download → tap), because MIUI gates adb installs: +# /sdcard/Download/PlainFiles-perf.apk (app under test, package …filemanager.perf) +# /sdcard/Download/PlainFiles-benchmark-test.apk + +./tools/benchmark-fixture.sh # create the fixture and grant storage access adb shell am instrument -w \ -e class de.softwareapp_hb.filemanager.benchmark.BrowserBenchmark \ de.softwareapp_hb.filemanager.benchmark/androidx.test.runner.AndroidJUnitRunner ``` -With a device that accepts installs, the Gradle path does the same and writes the numbers next to the -build output: - -```bash -./gradlew -PbenchmarkBuild :benchmark:connectedDebugAndroidTest -``` - -`-PbenchmarkBuild` is not decoration: it is what points the app's instrumentation at the `benchmark` -build type. Setting that unconditionally in the build file makes `benchmark` the variant *unit* tests -run against, and `:app:testDebugUnitTest` — the task the gate is built on — disappears. Measured, not -guessed. +`am instrument` is preferred over `./gradlew -PbenchmarkBuild :benchmark:connectedDebugAndroidTest` on a +device with an install gate: the Gradle task installs *and uninstalls* the APKs around the run, so every +run needs a human again — and an uninstall between runs is what made the app under test disappear once. ## Not measured yet, and why diff --git a/tools/benchmark-fixture.sh b/tools/benchmark-fixture.sh new file mode 100755 index 0000000..10da97d --- /dev/null +++ b/tools/benchmark-fixture.sh @@ -0,0 +1,43 @@ +#!/usr/bin/env bash +# +# Prepares the device for the macrobenchmark module, and takes it back afterwards. +# +# ./tools/benchmark-fixture.sh # create the folder, grant storage access +# ./tools/benchmark-fixture.sh --remove # delete the folder again +# +# Why this is a script rather than setup inside the benchmark: UiAutomation's executeShellCommand +# runs with the *test app's* uid, not the shell's, so it cannot write to /sdcard and cannot grant +# itself an app-op. The first version of the benchmark tried exactly that and failed with a +# RuntimeException from UiAutomationConnection, leaving no fixture and a hang. Through adb the same +# commands run as the shell user, which has the rights this needs. +# +# The folder is at the root of shared storage under a name that sorts before everything else, so the +# benchmark finds its row without scrolling. +set -euo pipefail + +package=de.softwareapp_hb.filemanager.perf +directory=/sdcard/AABenchmark +count=1000 + +case "${1:-create}" in + --remove) + adb shell "rm -rf $directory" + echo "benchmark-fixture: removed $directory" + ;; + create) + # One shell loop with builtin redirection: no fork per file, so a thousand files take a moment + # rather than a minute. + adb shell "rm -rf $directory; mkdir -p $directory; \ + i=0; while [ \$i -lt $count ]; do : > $directory/file-\$i.txt; i=\$((i+1)); done" + # The benchmark build is not the debug build, so it starts with no storage grant; granting it + # here is what lets the run be unattended. No `--uid`: that flag takes a uid, not a package + # name, and passing a package there grants nothing while still reporting that something was + # set — which is exactly the kind of half-truth that costs an afternoon. + adb shell "appops set $package MANAGE_EXTERNAL_STORAGE allow" + echo "benchmark-fixture: $count files in $directory, storage granted to $package" + ;; + *) + echo "usage: $0 [--remove]" >&2 + exit 2 + ;; +esac