BaselineProfileProducerPlugin.kt gives a misleading error about inability to determine the test task [313803289]

Fixed

Bug

Status Update

No update yet.

Description

ma...@textnow.com

created issue #1

Nov 29, 2023 05:38PM

DESCRIBE THE ISSUE IN DETAIL:

STEPS TO REPRODUCE:
1. Use the IDE to auto-generate a baseline profile module
2. Change _ONE_ of the two device references from pixel6 to pixel8, but not both (ie: we would expect an error message, but not this error message)
3. run `./gradlew tasks` and notice that it says it can not determine the test task. This leads devs down a rabbit hole trying to solve the wrong thing. Ideally, it would tell them there is a device mismatch.

Issue is coming from: BaselineProfileProducerPlugin.kt#createTasksForVariant

(I was not able to place this ticket in the correct component)

```
* What went wrong:
A problem occurred configuring project ':baselineprofile'.
> com.android.build.gradle.internal.crash.ExternalApiUsageException: org.gradle.api.GradleException: It wasn't possible to determine the test task for managed device `pixel8Api34`.
Please check the managed devices specified in the baseline profile
configuration.
```

Studio Build: Iguana 2023.2.1 Canary 15
Version of Gradle Plugin: 8.2.0-rc03
Version of Gradle: 8.3-rc-2-all
Version of Java: 17
OS: Ubuntu 22.04.3 LTS x86_64

Comments

ja...@google.com <ja...@google.com> #2Nov 30, 2023 05:51AM

Project: platform/frameworks/support
Branch: androidx-main

commit adc821747c2a7f56b6708344dcb0dcc1c5707c90
Author: Rahul Ravikumar <rahulrav@google.com>
Date: Tue Apr 26 13:55:46 2022

Workaround compilation reset unavailable on user builds

* cmd package compile --reset does not work on user builds.
* To work around this, we copy the apk to the tmp directory, and then kick off an un-install and a re-install.
* A side-effect of uninstalling / re-installing is that ART profiles end up in vfy, which is the state we expect.
* Added `compilation.enabled` as an instrumentation test argument, to disable this behavior if the benchmark wants
full control on CompilationModes.

Test: SmallListStartupBenchmark
Fixes:

b/230665435

Bug:

b/230518212

Relnote: "Fixed an issue where compiled methods were not correctly being reset between each macrobenchmark on unrooted builds. This unfortunately requires reinstalling the apk each iteration, which will clear application data for each macrobenchmark."

Before:

SmallListStartupBenchmark_startup[startup=COLD,compilation=None]
startupMs min 198.5, median 210.5, max 222.6
timeToInitialDisplayMs min 196.6, median 208.0, max 220.5

SmallListStartupBenchmark_startup[startup=COLD,compilation=BaselineProfile]
startupMs min 190.7, median 205.3, max 212.2
timeToInitialDisplayMs min 188.8, median 202.2, max 210.3

After the change:

SmallListStartupBenchmark_startup[startup=COLD,compilation=None]
startupMs min 223.9, median 230.1, max 243.8
timeToInitialDisplayMs min 222.1, median 228.3, max 242.0

SmallListStartupBenchmark_startup[startup=COLD,compilation=BaselineProfile]
startupMs min 205.2, median 213.5, max 228.7
timeToInitialDisplayMs min 203.3, median 211.7, max 227.0

Change-Id: I31c740019640da0d0913066bb9ac59ab5c2a9627

M benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/MacrobenchmarkScope.kt
M benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/CompilationMode.kt
M benchmark/benchmark-common/src/main/java/androidx/benchmark/Arguments.kt

https://android-review.googlesource.com/2076902

ma...@textnow.com <ma...@textnow.com> #3Nov 30, 2023 02:59PM

Should we fix --reset? I haven't tested, but it looks like it's just a matter of changing enforceSystemOrRoot to enforceSystemOrRootOrShell on the line Rahul indicated.

As Nicolas argues in http://b/230518212#comment9 this doesn't seem like a big security regression - at worst it may make apps slower, right? I see a similar permission given to force dexopting a package. Should we loop in someone from security team on this?

rk...@google.com <rk...@google.com> Dec 7, 2023 07:30PM

Assigned to an...@google.com.

je...@google.com <je...@google.com> Dec 12, 2023 05:24AM

Reassigned to ma...@google.com.

ma...@google.com <ma...@google.com> #4Dec 12, 2023 10:31PM

Yes, we should still fix --reset such that it works on user builds. Maybe allow the shell to clear caches, and not the app's themselves ? The goal is to be able to make benchmarks easier.

Message last modified on Dec 12, 2023 11:01PM

ap...@google.com <ap...@google.com> #5Jan 5, 2024 09:17PM

This bug is for the behavior in macrobenchmark and the workaround.

The issue of opening up that behavior is being tracked at b/230518212

ma...@google.com <ma...@google.com> Jan 5, 2024 11:28PM

Marked as fixed.

pr...@google.com <pr...@google.com> #6Feb 21, 2024 07:32PM

This fix made all compilation=none and compilation=warmup measurements slower in CI dashboard screenshot

E.g. androidx.compose.integration.macrobenchmark.SmallListStartupBenchmark_startup_startup_COLD_compilation_None_ went from 340 -> 380ms.

As these were rooted devices, this indicates --reset (done in our rooted CI) does not put the device into the same state as fully reinstallation (after CL landed).