docs: add AI-assisted code review and benchmark maintenance - #27
Merged
Conversation
runBenchmarkCompose/View/All depended on task paths that didn't exist (:benchmark:benchmarkComposeRun was never defined; benchmarkViewRun is registered on the root project, not :benchmark), so --dry-run failed with "Task not found". Add the missing benchmarkComposeRun task and correct the task references throughout.
…enchmarks sanitizedMetricsForBenchmark()/measureStartupWithCapabilityReport() lived in benchmark/src/androidTest/.../BenchmarkUtils.kt, which is dead code for this com.android.test module (only src/main is compiled into the instrumentation APK) and was never called from anywhere reachable. It also imported androidx.benchmark.macro.MacrobenchmarkRule instead of the .junit4 variant actually used by the test classes -- likely why it was never wired in to begin with. Move the capability-report logic into the real src/main/BenchmarkUtils.kt with the import fixed, add the kotlinx-serialization-json dependency the JSON logging needs (:shared only exposes it as `implementation`, not transitive), and switch coldStartup_compose()/coldStartup_view() to call measureStartupWithCapabilityReport() instead of hardcoding a metrics list. Delete the now-redundant dead file. Fast-scroll tests and SmokeBenchmark.kt are left as-is; their custom swipe measureBlock doesn't fit this cold-start-shaped helper.
results/README.md's stated policy is curated summaries/manifests only, but the raw *-benchmarkData.json and additionaltestoutput.*.txt dumps were sitting untracked in results/ with nothing stopping a future `git add -A` from committing ~370KB of raw output. Add explicit ignore patterns so the policy actually holds.
…07-06) README.md and results/run-manifest-2026-08-07.md cited an emulator run on 2026-08-07, committed by f582b68 on 2026-07-09 -- a month before its own claimed run date. The raw perfetto-trace filenames and artifact mtimes retained under results/ show the run actually happened on 2026-07-06. Rename the manifest and update references accordingly; the committed numbers are unchanged, only the date/filenames were wrong. Also cross-reference this run against benchmark-results.md's separate 2026-07-23 run so the two results docs read as distinct dated data points instead of unrelated, conflicting claims.
Mark the Gradle task graph, MetricCapability wiring, results artifact policy, and run-date/provenance findings as resolved with what changed. Keep the exception-handling, nested-Gradle-invocation, and emulator-only findings open. Also correct the coldStartup_view() test description (no longer StartupTimingMetric-only) and flag a similarly-shaped forward-dated/inconsistent-host issue found in README's "Maintenance status" header that wasn't independently verified this pass.
…K-path claim Same source commit (f582b68) that mislabeled the benchmark run date also changed this header from 2026-05-31 to 2026-08-07, a date in the future relative to its own 2026-07-09 commit timestamp. Corrected to 2026-07-09, the only real evidence of when the text was written. Also drop the claim that local.properties points at a nonexistent macOS SDK path: local.properties is gitignored and per-developer by design, so it shouldn't have been asserted as a persistent repo fact -- and it's currently false in this environment (valid Linux SDK path; every Gradle command this session succeeded).
…arity app-view.MainActivity extended AppCompatActivity while app-compose.MainActivity uses the lighter ComponentActivity -- a real confound in a study whose core claim depends on a fair cold-start comparison (tracked as TODO #6 in benchmark-results.md). Nothing here uses AppCompat delegate behaviors (no ActionBar/Toolbar, no vector-drawable back-compat needed at minSdk 24), so Theme.MaterialComponents.DayNight.NoActionBar still resolves without it. Verified assembleBenchmark/assembleRelease (incl. lintVitalRelease) pass. Not re-benchmarked on a device -- the published cold-start numbers in benchmark-results.md/README.md were measured against the old AppCompatActivity version and may no longer be accurate.
fastScroll_compose() and fastScroll_view() had byte-for-byte identical swipe-gesture logic inlined in each measureBlock. Extract to BenchmarkUtils.performFastScrollGestures(). Pure code motion, no behavior change.
… steps Record the maintenance-status header fix, the ComponentActivity parity fix, the shared gesture-helper extraction, and a warning about an incorrect DiffUtil recommendation in the untracked REVIEW.md found in this checkout. Add re-benchmark-after-ComponentActivity-change and the Thread.sleep gesture timing to the still-open list.
- CLAUDE.md: remove the "Known broken tasks" note for runBenchmarkCompose/View/All -- this PR fixes those tasks, so the note now contradicted the "Resolved" section later in the same file. - BenchmarkUtils.kt: update the class KDoc, which still said "minimal and free of instrumentation-specific side-effects" despite now containing Logcat capability-report emission and instrumentation-dependent helpers.
Pulls together what's left after this PR's audit pass into one prioritized, actionable list (P0: needs a device/emulator run; P1: safe code fixes; P2: research-quality polish; P3: minor). Matches the NEXT_STEPS.md convention already used elsewhere in this workspace (see JMW/game/NEXT_STEPS.md).
…ance project - Architecture review (4 modules, ~2,500 LOC) - Module-by-module findings and recommendations
There was a problem hiding this comment.
Pull request overview
This PR is a maintenance + documentation pass for the Compose-vs-Views performance benchmark project: it updates benchmark methodology docs/results provenance, and refactors the benchmark module to support capability-aware metric selection while keeping Compose/View benchmarks comparable.
Changes:
- Added/updated project docs (AI-assisted review notes + consolidated next steps) and corrected run-date/maintenance-status references in results docs.
- Refactored Macrobenchmark code to (a) capability-sanitize requested cold-start metrics and (b) dedupe fast-scroll gestures into a shared helper.
- Updated benchmark convenience tasks and gitignore rules to align with the repo’s “curated summaries only” results artifact policy.
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| REVIEW.md | New project review document (needs sync with current code per comments). |
| NEXT_STEPS.md | New consolidated roadmap for remaining benchmark work. |
| README.md | Corrected maintenance header + updated references to the corrected run manifest/date. |
| results/run-manifest-2026-07-06.md | Corrected mislabeled run date and artifact paths; should align wording with results artifact policy. |
| CLAUDE.md | Updated benchmark source-set guidance + added audit/known-issues notes. |
| .gitignore | Ignores raw per-run benchmark artifacts (policy enforcement). |
| build.gradle.kts | Adjusted benchmark convenience tasks and wiring. |
| benchmark/src/main/java/dev/egarcia/andperf/benchmark/BenchmarkUtils.kt | Added capability-report helpers + extracted fast-scroll gestures. |
| benchmark/src/main/java/dev/egarcia/andperf/benchmark/ComposeBenchmarks.kt | Cold-start now uses capability-aware metric selection; fast-scroll uses shared gesture helper. |
| benchmark/src/main/java/dev/egarcia/andperf/benchmark/ViewBenchmarks.kt | Same as Compose: capability-aware cold-start + shared gesture helper. |
| benchmark/build.gradle.kts | Adds direct kotlinx-serialization-json dependency for capability JSON logging. |
| benchmark-results.md | Notes earlier run and marks ComponentActivity parity task completed. |
| app-view/src/main/java/dev/egarcia/andperf/view/MainActivity.kt | Switched from AppCompatActivity → ComponentActivity for parity with Compose. |
| benchmark/src/androidTest/java/dev/egarcia/andperf/benchmark/BenchmarkUtils.kt | Deleted dead/uncompiled legacy BenchmarkUtils. |
Suppressed comments (3)
results/run-manifest-2026-07-06.md:55
- The section title “Retained Artifacts” implies the raw JSON/text files are present in the repo, but
results/README.mddefines that raw artifacts stay out of Git (and this checkout doesn’t contain the referencedresults/run-2026-07-06-*files). Consider rewording to “artifact locations” to match the policy.
results/run-manifest-2026-07-06.md:117 - This summary says the raw JSON/text artifacts “are retained under
results/and serve as the single source of truth”, but perresults/README.mdthose raw artifacts aren’t tracked in Git. The manifest itself should be described as the tracked source of truth, with the raw files being external retention locations.
REVIEW.md:87 - This benchmark review section is now out of sync with the code in this PR: the
Thread.sleep(150)pacing is no longer “line 62 of both benchmark files” (it lives inBenchmarkUtils.performFastScrollGestures()), and the gesture duplication called out as a minor issue has already been addressed by extracting that helper.
| **Fixed in PR #25:** Now uses both `StartupTimingMetric` and `FrameTimingMetric` ✅ | **P0 — `Thread.sleep(150)` in swipe gesture loop** (line 62 of both benchmark files) — non-deterministic, could cause flaky results. Consider `GestureTimeoutDetector` or disabling animation. |
| Properly skips tests if package not installed (`BenchmarkUtils.isPackageInstalled`) | **P1 — Swipe coordinates in raw pixels** (e.g. `startX = (width * 0.5).toInt()`) — device-dependent (ok for benchmark, but note it). |
| Graceful error handling: `catch { Assume.assumeTrue(...) }` | **P1 — 8 swipes at 150ms = 1200ms of sleep** per iteration. Total benchmark time per test is ~10–15 seconds. Acceptable but could be faster. |
| Self-instrumenting enabled (`android.experimental.self-instrumenting = true`) | |
| | Minor: `fastScroll_view()` and `fastScroll_compose()` use identical gesture patterns — fine for comparison, but consider extracting the gesture logic to avoid duplication. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - Benchmark classes currently present in the project are `ComposeBenchmarks` and `ViewBenchmarks` under `benchmark/src/main/java/dev.egarcia.andperf.benchmark/`. | ||
| - Local Gradle verification from this maintenance environment is blocked until Android SDK configuration is corrected (`local.properties` points to `/Users/egarcia/Library/Android/sdk`, which does not exist on this Linux host). | ||
| - The README example results table is now **populated with verified benchmark data** from an emulator run on 2026-08-07 (Android 16, API 36). Raw artifacts are retained under `results/` — see [`results/run-manifest-2026-08-07.md`](results/run-manifest-2026-08-07.md). | ||
| - The README example results table is now **populated with verified benchmark data** from an emulator run on 2026-07-06 (Android 16, API 36) — corrected 2026-07-23 from a previously mislabeled "2026-08-07" date; see the manifest's correction note. Raw artifacts are retained under `results/` — see [`results/run-manifest-2026-07-06.md`](results/run-manifest-2026-07-06.md). |
Comment on lines
+48
to
+52
| | Programmatic layout (no XML overhead) | **P0 — No `DiffUtil` or `ListAdapter`** — 1000 items rebind every change, wasting CPU. Switch to `ListAdapter` or `DiffUtil`. | | ||
| | Clean view holder pattern | **P1 — No `RecyclerView.RecycledViewPool`** — 1000 items without a pool may cause layout inflation overhead. | | ||
| | Constructor-injected data | **P1 — No scroll config** (e.g. `setNestedScrolling(false)`) — could add gesture conflicts if expanded. | | ||
| | | **P2 — `AppCompatActivity` vs Compose's `ComponentActivity`** — tracked as TODO #6 in benchmark-results.md. | | ||
| | | Minor: no `RecyclerView.ItemDecoration` for spacing. | |
Comment on lines
+8
to
+13
| - PR [#26](https://github.com/e-Garcia/Compose-vs-Android-View-System-Performance/pull/26) | ||
| is open against `main` with a maintenance-audit pass: fixed Gradle task graph, wired the | ||
| MetricCapability architecture into cold-start benchmarks, corrected a mislabeled run date | ||
| and a stale maintenance-status header, fixed a `MainActivity` base-class parity gap | ||
| (`AppCompatActivity` → `ComponentActivity`), deduped fast-scroll gesture code, and addressed | ||
| Copilot's review feedback on the PR itself. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (3)
README.md:17
- This claims raw benchmark artifacts are “retained under
results/”, butresults/README.mddescribes that raw per-run artifacts should stay out of Git and only curated manifests/summaries are tracked. Consider clarifying that raw artifacts are retained locally (and are gitignored) to avoid implying they’re present in a fresh clone.
- Benchmark classes currently present in the project are `ComposeBenchmarks` and `ViewBenchmarks` under `benchmark/src/main/java/dev.egarcia.andperf.benchmark/`.
- The README example results table is now **populated with verified benchmark data** from an emulator run on 2026-07-06 (Android 16, API 36) — corrected 2026-07-23 from a previously mislabeled "2026-08-07" date; see the manifest's correction note. Raw artifacts are retained under `results/` — see [`results/run-manifest-2026-07-06.md`](results/run-manifest-2026-07-06.md).
- NOTE: Results are from an emulator, not a physical device. Physical-device runs should be performed before relying on these numbers for production decisions. See device note in the results section.
REVIEW.md:156
- This section is internally inconsistent: it says Copilot cannot be requested via
gh pr edit --add-reviewer <name>, but later provides a workinggh pr edit ... --add-reviewer copilot-pull-request-reviewercommand. Reword to clarify that thecopilotlogin doesn’t exist, whilecopilot-pull-request-reviewerdoes (when the feature is enabled).
Copilot cannot be requested via `gh pr edit --add-reviewer <name>` — GitHub does not expose Copilot as a user login. Here are the working alternatives:
benchmark/src/main/java/dev/egarcia/andperf/benchmark/BenchmarkUtils.kt:75
- The KDoc says “Unsupported metric families are excluded before measurement”, but the implementation currently throws for any
MetricTypeother thanSTARTUP/FRAME_TIMING. Either handle unsupported types by excluding them (and reflecting that in the capability report), or update the KDoc to match the current behavior to avoid misleading future callers.
/**
* Converts requested metric families to concrete Macrobenchmark metrics after emitting a
* structured capability report. Unsupported metric families are excluded before measurement.
*/
fun sanitizedMetricsForBenchmark(
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (3)
benchmark/src/main/java/dev/egarcia/andperf/benchmark/BenchmarkUtils.kt:88
- The KDoc says unsupported metric families are "excluded before measurement", but
sanitizedMetricsForBenchmark()currently throws for anyMetricTypeother than STARTUP/FRAME_TIMING. Either update the docs or actually ignore unsupported types to match the stated behavior.
/**
* Converts requested metric families to concrete Macrobenchmark metrics after emitting a
* structured capability report. Unsupported metric families are excluded before measurement.
*/
fun sanitizedMetricsForBenchmark(
CLAUDE.md:78
- This section mentions a leftover
benchmark/src/androidTest/.../BenchmarkUtils.ktfile that "is not part of the build", but that file has been deleted in this PR. Update the note so it doesn't point readers at a non-existent path.
There is a leftover `benchmark/src/androidTest/java/.../BenchmarkUtils.kt` file that is **not** part of the build for this module (dead code from an earlier metric-capability refactor — see Known Issues below). Do not edit it expecting it to affect test behavior; edit `benchmark/src/main/.../BenchmarkUtils.kt` instead.
REVIEW.md:155
- This paragraph is internally inconsistent: it says Copilot can't be requested via
gh pr edit --add-reviewer, but later in the same section recommendsgh pr edit ... --add-reviewer copilot-pull-request-reviewer. Reword the intro sentence to match the rest of the section.
Copilot cannot be requested via `gh pr edit --add-reviewer <name>` — GitHub does not expose Copilot as a user login. Here are the working alternatives:
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (9)
benchmark/src/main/java/dev/egarcia/andperf/benchmark/BenchmarkUtils.kt:97
report.availableis mapped with anelse -> error(...), which contradicts the stated "unsupported metric families are excluded" behavior and makes capability reporting fragile if the probe ever returns additionalMetricTypes.
return report.available.map { metric ->
when (metric) {
MetricType.STARTUP -> StartupTimingMetric()
MetricType.FRAME_TIMING -> FrameTimingMetric()
else -> error("No Macrobenchmark metric mapping exists for $metric")
results/run-manifest-2026-07-06.md:57
- This report states raw artifacts are "retained" under
results/, but the repo now gitignores these paths and they are not present in version control. The manifest should explicitly say these files are not committed and are only local retention locations (or point to where they can be fetched).
This issue also appears on line 115 of the same file.
results/run-manifest-2026-07-06.md:117
- The summary claims the raw JSON/text artifacts "are retained under
results/" and are the single source of truth, but those files are not checked in (and are now covered by.gitignore). This should be reworded to avoid implying readers can find the raw artifacts in the repo.
README.md:16 - This bullet says "Raw artifacts are retained under
results/", but the raw per-run files are intentionally not committed (and are now gitignored). Consider clarifying that only the manifest is tracked, and raw artifacts must be reproduced or fetched from the run environment.
- The README example results table is now **populated with verified benchmark data** from an emulator run on 2026-07-06 (Android 16, API 36) — corrected 2026-07-23 from a previously mislabeled "2026-08-07" date; see the manifest's correction note. Raw artifacts are retained under `results/` — see [`results/run-manifest-2026-07-06.md`](results/run-manifest-2026-07-06.md).
REVIEW.md:48
- This
DiffUtil/ListAdapterrecommendation appears incorrect for the current code:MainActivityconstructsItemAdapter(FakeRepo.items())once with an immutable list and never updates it (no subsequent notify/submit calls), so there is no "rebind every change" scenario in this benchmark.
| Programmatic layout (no XML overhead) | **P0 — No `DiffUtil` or `ListAdapter`** — 1000 items rebind every change, wasting CPU. Switch to `ListAdapter` or `DiffUtil`. |
benchmark/src/main/java/dev/egarcia/andperf/benchmark/BenchmarkUtils.kt:87
- The KDoc says unsupported metric families are excluded, but this mapping throws for any
MetricTypeother than STARTUP/FRAME_TIMING. That makes the helper crash if callers ever pass POWER/MEMORY/THERMAL/NETWORK (which are valid enum values).
This issue also appears on line 93 of the same file.
val requirements = requestedMetrics.map { metric ->
when (metric) {
MetricType.STARTUP -> MetricRequirements.Startup
MetricType.FRAME_TIMING -> MetricRequirements.FrameTiming
else -> error("No Macrobenchmark metric mapping exists for $metric")
REVIEW.md:156
- These lines contradict each other: the section claims Copilot cannot be requested via
gh pr edit --add-reviewer ..., but later provides a workinggh pr edit ... --add-reviewer copilot-pull-request-reviewerexample. This should be reconciled to avoid confusing readers.
Copilot cannot be requested via `gh pr edit --add-reviewer <name>` — GitHub does not expose Copilot as a user login. Here are the working alternatives:
REVIEW.md:188
- The
gh api .../branches/main/reviewsendpoint in Option C is not a valid GitHub REST API endpoint for branch protection / required reviews, so these commands will fail or mislead readers. Prefer linking to the official branch protection / rulesets docs instead of an incorrect command sequence.
# Add required review from Copilot via the REST API
gh api repos/e-Garcia/Compose-vs-Android-View-System-Performance/branches/main/reviews \
--method POST --field reviewer='Copilot'
**CLAUDE.md:78**
* This mentions a leftover `benchmark/src/androidTest/.../BenchmarkUtils.kt` file, but that file is deleted in this PR. Keeping this note as-is will confuse readers about where benchmark code lives.
IMPORTANT: Benchmark tests actually live in benchmark/src/main/java/dev/egarcia/andperf/benchmark/ (this is a com.android.test module, so its "main" source set — not androidTest — is what gets compiled into the instrumentation APK). The runnable classes are ComposeBenchmarks.kt, ViewBenchmarks.kt, and SmokeBenchmark.kt.
There is a leftover benchmark/src/androidTest/java/.../BenchmarkUtils.kt file that is not part of the build for this module (dead code from an earlier metric-capability refactor — see Known Issues below). Do not edit it expecting it to affect test behavior; edit benchmark/src/main/.../BenchmarkUtils.kt instead.
</details>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Files changed (14 files, +651/-222):
Test Plan