diff --git a/AGENTS.md b/AGENTS.md index 2de9dfe..a800bf7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -28,6 +28,8 @@ GRADLE_FLAGS=--offline ./tools/verify.sh # when the network is unavailable ./gradlew :app:assembleDebug # then: adb install -r app/build/outputs/apk/debug/app-debug.apk ./tools/measure-startup.sh 5 # cold start, median of five, on the installed device +./gradlew :benchmark:connectedDebugAndroidTest # the four macrobenchmark tests (needs a device) + # add -PbenchmarkBuild: it targets the benchmark build ./gradlew :app:detektBaseline # re-freezes the findings that exist now — think before running it python3 tools/make-code-map.py # regenerate docs/CODE_MAP.md after touching source ``` diff --git a/CHANGELOG.md b/CHANGELOG.md index 0060550..f339112 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -186,8 +186,17 @@ upload needs a **higher `versionCode`** than every previous upload. 5 000 / 20 000 files, thumbnail paint while scrolling, and keystroke-to-result — with the method each would use and the honest reason they are missing: they need a `:benchmark` module and a profileable build under the release package id, and this device waits for a confirmation dialog on the first - install of a new package id. The module is deliberately absent rather than half-configured: build - wiring nobody can run is worse than a recorded gap. + install of a new package id. The module now exists — `benchmark/`, four macrobenchmark tests, its own + `.perf` package id so it cannot collide with the test APK or with a release install — and + `tools/verify.sh --release` compiles it so it cannot rot between device runs. What it has not done is + produce a number: on this phone `adb install` of a new package id is refused with + `INSTALL_FAILED_USER_RESTRICTED`, so the two APKs are pushed to `/sdcard/Download/` for a manual + install and the run is waiting on that. Two build facts had to be learned the hard way and are + written next to the code: the app under test needs its own package id (`.perf`), because the + benchmark module's own APK already owns `.benchmark` and an app with that id collides with it; and + `testBuildType` is behind a `-PbenchmarkBuild` flag, because setting it unconditionally makes + `benchmark` the variant *unit* tests run against and `:app:testDebugUnitTest` — the task this + project's whole gate is built on — stops existing. ### Changed diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 669f63c..5b2797f 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -95,6 +95,26 @@ android { "proguard-rules.pro", ) } + /* + * What macrobenchmark measures: release rules (R8 on, not debuggable) but signed with the + * debug key so it can be installed on a test device. The manifest in src/benchmark adds the + * one thing a benchmark cannot work without — ``. + * + * The suffix is not cosmetic, and it is `.perf` rather than `.benchmark` on purpose: the + * benchmark module's own APK is `de.softwareapp_hb.filemanager.benchmark`, so an app under + * test with that id collides with the test APK and the install fails with + * INSTALL_FAILED_VERSION_DOWNGRADE. Without a suffix at all, this build would have the app's + * own package id and a debug signature — which cannot be installed beside a release build, + * and would replace a release install if the keys happened to match. A benchmark must never be + * able to take over the shipped app. + */ + create("benchmark") { + initWith(getByName("release")) + applicationIdSuffix = ".perf" + signingConfig = signingConfigs.getByName("debug") + matchingFallbacks += listOf("release") + isDebuggable = false + } } compileOptions { @@ -102,6 +122,20 @@ android { targetCompatibility = JavaVersion.VERSION_17 } + /* + * Which build type the instrumentation tests run against — only when asked for. + * + * Setting this unconditionally also makes `benchmark` the variant that *unit* tests run against, + * and `:app:testDebugUnitTest` — the task the gate and CI are built on — stops existing. That is + * measured rather than guessed: with it set, the only unit-test task left was + * `testBenchmarkUnitTest`. + * + * ./gradlew -PbenchmarkBuild :benchmark:connectedDebugAndroidTest + */ + if (providers.gradleProperty("benchmarkBuild").isPresent) { + testBuildType = "benchmark" + } + buildFeatures { compose = true } diff --git a/app/src/benchmark/AndroidManifest.xml b/app/src/benchmark/AndroidManifest.xml new file mode 100644 index 0000000..59e9b7b --- /dev/null +++ b/app/src/benchmark/AndroidManifest.xml @@ -0,0 +1,17 @@ + + + + + + + diff --git a/benchmark/build.gradle.kts b/benchmark/build.gradle.kts new file mode 100644 index 0000000..65d6e9b --- /dev/null +++ b/benchmark/build.gradle.kts @@ -0,0 +1,41 @@ +plugins { + // No version: `com.android.application` in :app has already put AGP on the build's plugin + // classpath, and Gradle refuses a second request for a version of it ("already on the classpath + // with an unknown version"). The version is still the one in the catalog — see libs.versions.toml. + id("com.android.test") +} + +/** + * The macrobenchmark module: a test APK that instruments the app rather than shipping inside it. + * + * It has no production code and no build types of its own — it points at `:app`'s `benchmark` build + * type through `targetProjectPath` and instruments that. `tools/verify.sh --release` compiles it so + * that a rename in the app cannot leave these benchmarks quietly broken between device runs. + */ +android { + namespace = "de.softwareapp_hb.filemanager.benchmark" + compileSdk = 36 + + defaultConfig { + minSdk = 26 + targetSdk = 36 + testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner" + } + + targetProjectPath = ":app" + // The benchmark itself is the thing being instrumented: it starts the app under test, so it must + // not be run as a normal instrumentation target. + experimentalProperties["android.experimental.self-instrumenting"] = true + + compileOptions { + sourceCompatibility = JavaVersion.VERSION_17 + targetCompatibility = JavaVersion.VERSION_17 + } +} + +dependencies { + implementation(libs.androidx.benchmark.macro.junit4) + implementation(libs.androidx.test.ext.junit) + implementation(libs.androidx.test.uiautomator) + implementation(libs.androidx.test.runner) +} diff --git a/benchmark/src/main/AndroidManifest.xml b/benchmark/src/main/AndroidManifest.xml new file mode 100644 index 0000000..5b767b9 --- /dev/null +++ b/benchmark/src/main/AndroidManifest.xml @@ -0,0 +1,3 @@ + + + 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 new file mode 100644 index 0000000..36b49f5 --- /dev/null +++ b/benchmark/src/main/java/de/softwareapp_hb/filemanager/benchmark/BrowserBenchmark.kt @@ -0,0 +1,196 @@ +package de.softwareapp_hb.filemanager.benchmark + +import androidx.benchmark.macro.CompilationMode +import androidx.benchmark.macro.FrameTimingMetric +import androidx.benchmark.macro.MacrobenchmarkScope +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 +import org.junit.runner.RunWith + +/** + * The four numbers [docs/PERFORMANCE.md] promises, measured on a benchmark build of the app. + * + * ./gradlew :benchmark:connectedBenchmarkAndroidTest + * + * Three decisions are worth knowing before reading the output. + * + * **`CompilationMode.None()`**: no compilation profile is applied, so these are the numbers of code + * that has never run. That is the honest baseline for "is this fast on a cold install", and it is + * also the mode that does not need a profile file to exist. `CompilationMode.Partial()` — what a + * baseline profile would buy — is the follow-up, and it needs the profile generated first. + * + * **Frames, not milliseconds.** Three of the four use [FrameTimingMetric], because what a user calls + * slow is jank on screen, not a state transition finishing. The alternative is a named trace section + * inside the app; that would measure "tap to rows ready" more directly, and it would put benchmarking + * calls into production code for the sake of a number. + * + * **The device has to be in English**, because the navigation is driven by the labels on screen — + * "Internal storage", "Search this folder". A localised device fails with a message saying so rather + * than silently measuring something else. + */ +@RunWith(AndroidJUnit4::class) +class BrowserBenchmark { + + @get:Rule + val benchmark = MacrobenchmarkRule() + + /** + * The app under test: the `benchmark` build type, which carries its own package id (`.perf`) so it + * can never collide with — or replace — a release install on the device, or with the test APK. + */ + private val app = "de.softwareapp_hb.filemanager.perf" + + @Test + fun coldStart() = benchmark.measureRepeated( + packageName = app, + metrics = listOf(StartupTimingMetric()), + compilationMode = CompilationMode.None(), + iterations = 5, + startupMode = StartupMode.COLD, + ) { + pressHome() + startActivityAndWait() + } + + @Test + fun openAFolderOfAThousandFiles() = benchmark.measureRepeated( + packageName = app, + metrics = listOf(FrameTimingMetric()), + compilationMode = CompilationMode.None(), + iterations = 5, + setupBlock = { openFixture(device) }, + ) { + device.findObject(By.text(Fixture.NAME))!!.click() + device.wait(Until.hasObject(By.text(Fixture.FIRST_FILE)), TIMEOUT_MS) + } + + @Test + fun scrollAFolderOfAThousandFiles() = benchmark.measureRepeated( + packageName = app, + metrics = listOf(FrameTimingMetric()), + compilationMode = CompilationMode.None(), + iterations = 5, + setupBlock = { + openFixture(device) + device.findObject(By.text(Fixture.NAME))!!.click() + device.wait(Until.hasObject(By.text(Fixture.FIRST_FILE)), TIMEOUT_MS) + }, + ) { + repeat(4) { + device.swipe( + device.displayWidth / 2, + (device.displayHeight * 3) / 4, + device.displayWidth / 2, + device.displayHeight / 4, + 20, + ) + } + } + + @Test + fun searchInsideAFolderOfAThousandFiles() = benchmark.measureRepeated( + packageName = app, + metrics = listOf(FrameTimingMetric()), + compilationMode = CompilationMode.None(), + iterations = 5, + setupBlock = { + openFixture(device) + device.findObject(By.text(Fixture.NAME))!!.click() + device.wait(Until.hasObject(By.text(Fixture.FIRST_FILE)), TIMEOUT_MS) + }, + ) { + device.findObject(By.desc(SEARCH_ACTION))!!.click() + val field = device.wait(Until.findObject(By.clazz("android.widget.EditText")), TIMEOUT_MS) + assertNotNull("the search field should open from the app bar", field) + field!!.text = "file-99" + device.wait(Until.hasObject(By.text("file-99.txt")), TIMEOUT_MS) + } + + // ------------------------------------------------------------------ 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. + */ + private fun MacrobenchmarkScope.openFixture(device: UiDevice) { + Fixture.ensure(device, app) + pressHome() + startActivityAndWait() + val volume = device.wait(Until.findObject(By.text("Internal storage")), TIMEOUT_MS) + assertNotNull( + "the volume row is not on screen. Is the device in English, and did the storage grant " + + "land? The benchmark grants it with `appops set` before navigating.", + volume, + ) + volume!!.click() + assertNotNull( + "the fixture folder is not in the volume root, so setup did not complete", + device.wait(Until.findObject(By.text(Fixture.NAME)), TIMEOUT_MS), + ) + } + + companion object { + private const val TIMEOUT_MS = 10_000L + + /** 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. + * + * 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 + * UI changes. + */ +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 7c609cd..39ba4d5 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. -76 source files, 12492 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, 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. 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`. @@ -164,6 +164,14 @@ Compose theme, colours, type | `FileManagerApplication.kt` | 126 | Application entry point and the app's tiny object graph. FileManagerApplication | - | | `MainActivity.kt` | 65 | MainActivity | - | +## benchmark/ + +The macrobenchmark test APK: four measurements over a fixture folder, run on a device + +| 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 | - | + ## Behaviour index Every test name in the suite, by file: the fastest way to see whether something is already covered, and what the app claims to do. diff --git a/docs/PERFORMANCE.md b/docs/PERFORMANCE.md index 4b44e61..a38a721 100644 --- a/docs/PERFORMANCE.md +++ b/docs/PERFORMANCE.md @@ -39,6 +39,49 @@ ours are mechanical to move and neither is fixed yet: the cache directory is onl copies something, and the free-space line on the roots screen can arrive a frame later. They are the first items of the speed work. +## Measured on demand: the `:benchmark` module + +`benchmark/` holds four macrobenchmark tests that measure the interaction numbers above on a benchmark +build of the app: release rules (R8 on, not debuggable), debug-signed, and profileable — under its own +package id, `…filemanager.perf`, so it can never collide with the test APK or take over a release +install on the device. + +| benchmark | metric | what it answers | +|---|---|---| +| `coldStart` | `StartupTimingMetric` | tap → first frame | +| `openAFolderOfAThousandFiles` | `FrameTimingMetric` | jank while a 1 000-file folder fills in | +| `scrollAFolderOfAThousandFiles` | `FrameTimingMetric` | jank while scrolling that folder | +| `searchInsideAFolderOfAThousandFiles` | `FrameTimingMetric` | jank while typing into search | + +They share a fixture — `/sdcard/AABenchmark`, a thousand empty files, created once per run and deleted +in `@AfterClass` — and the storage grant is applied with `appops`, so an unattended run is possible. +`CompilationMode.None()` is deliberate: no profile is applied, which is the honest cold-install +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: + +```bash +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. + ## Not measured yet, and why | number | how it would be taken | why it is not here | diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 182dbae..f79de04 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -42,6 +42,13 @@ robolectric = "4.17" # obvious in a heap dump. Never shipped — `debugImplementation` below. leakcanary = "2.14" +# Macrobenchmark (the `:benchmark` module). Its own versions because it is a test APK that is never +# part of the app. +benchmark = "1.5.0" +androidxTestJunit = "1.3.0" +uiautomator = "2.4.0" +androidxTestRunner = "1.7.0" + [libraries] androidx-core-ktx = { group = "androidx.core", name = "core-ktx", version.ref = "coreKtx" } androidx-documentfile = { group = "androidx.documentfile", name = "documentfile", version.ref = "documentfile" } @@ -80,6 +87,12 @@ robolectric = { group = "org.robolectric", name = "robolectric", version.ref = " androidx-compose-ui-test-junit4 = { group = "androidx.compose.ui", name = "ui-test-junit4" } androidx-compose-ui-test-manifest = { group = "androidx.compose.ui", name = "ui-test-manifest" } leakcanary-android = { group = "com.squareup.leakcanary", name = "leakcanary-android", version.ref = "leakcanary" } + +# Macrobenchmark, for the `:benchmark` module only. +androidx-benchmark-macro-junit4 = { group = "androidx.benchmark", name = "benchmark-macro-junit4", version.ref = "benchmark" } +androidx-test-ext-junit = { group = "androidx.test.ext", name = "junit", version.ref = "androidxTestJunit" } +androidx-test-uiautomator = { group = "androidx.test.uiautomator", name = "uiautomator", version.ref = "uiautomator" } +androidx-test-runner = { group = "androidx.test", name = "runner", version.ref = "androidxTestRunner" } kotlinx-coroutines-test = { group = "org.jetbrains.kotlinx", name = "kotlinx-coroutines-test", version.ref = "coroutines" } # Static analysis, run by the :app:detekt task from its own configuration — never an app dependency. diff --git a/settings.gradle.kts b/settings.gradle.kts index 97a2c39..d1d4555 100644 --- a/settings.gradle.kts +++ b/settings.gradle.kts @@ -22,3 +22,6 @@ dependencyResolutionManagement { rootProject.name = "FileManager" include(":app") +// The macrobenchmark test APK, on its own module because it instruments the app rather than shipping +// inside it. `tools/verify.sh --release` compiles it, so it cannot rot unnoticed between runs. +include(":benchmark") diff --git a/tools/make-code-map.py b/tools/make-code-map.py index 51670b9..468e680 100644 --- a/tools/make-code-map.py +++ b/tools/make-code-map.py @@ -49,6 +49,16 @@ PACKAGES = { "": "Application entry points", } +# Source outside app/src/main. The benchmark module is source too, and a map that silently omits a +# module is a map that lies by omission. (name in the map, path from the repository root, blurb) +EXTRA_ROOTS = [ + ( + "benchmark/", + "benchmark/src/main/java/de/softwareapp_hb/filemanager/benchmark", + "The macrobenchmark test APK: four measurements over a fixture folder, run on a device", + ), +] + DECLARATION = re.compile( r"^(?:(?:public|internal|private|protected|abstract|open|sealed|data|value|inline|" r"suspend|operator|override|const|lateinit|external|annotation|enum)\s+)*" @@ -187,6 +197,8 @@ def render(root: Path) -> str: test_dir = root / "app/src/test/java" / PACKAGE sources = scan_source(main_dir) tests = scan_tests(test_dir) + extras = [(name, blurb, scan_source(root / path)) for name, path, blurb in EXTRA_ROOTS] + extra_files = [entry for _, _, entries in extras for entry in entries] by_declaration = {} by_stem = {} @@ -218,7 +230,8 @@ def render(root: Path) -> str: "%d source files, %d lines - %d test files, %d 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.\n" - % (len(sources), sum(entry["lines"] for entry in sources), + % (len(sources) + len(extra_files), + sum(entry["lines"] for entry in sources) + sum(entry["lines"] for entry in extra_files), len(tests), sum(len(test["tests"]) for test in tests)), "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`.\n", @@ -245,6 +258,21 @@ def render(root: Path) -> str: % (Path(entry["rel"]).name, entry["lines"], cell.replace("|", "\\|").strip(), covered)) + for label, blurb, entries in extras: + if not entries: + continue + out.append("\n## %s\n" % label) + out.append("%s\n" % blurb) + out.append("\n| file | lines | what it holds | tests |") + out.append("|---|---|---|---|") + for entry in entries: + declared = ", ".join(name + "()" if kind == "fun" else name + for kind, name, doc, line in entry["decls"]) + cell = "%s%s" % ("%s " % entry["doc"] if entry["doc"] else "", declared) + out.append("| `%s` | %d | %s | - |" + % (Path(entry["rel"]).name, entry["lines"], + cell.replace("|", "\\|").strip())) + out.append("\n## Behaviour index\n") out.append("Every test name in the suite, by file: the fastest way to see whether " "something is already covered, and what the app claims to do.\n") diff --git a/tools/verify.sh b/tools/verify.sh index 335b188..0839a44 100755 --- a/tools/verify.sh +++ b/tools/verify.sh @@ -45,6 +45,11 @@ if $release; then # runs, and a release-only defect (a class reached only through a resource, for instance) is # exactly what this step is for. gradle :app:assembleRelease + + step "Benchmark APKs compile" + # No device: this only proves that :benchmark still compiles against the app's benchmark build + # type, which is the thing that rots silently between device runs. + gradle :app:assembleBenchmark :benchmark:assembleDebug fi printf '\n\033[1mverify: all checks passed\033[0m\n'