Stabilize measurements for CompilationMode.None [230665435]

Fixed

Bug

Status Update

No update yet.

Description

ra...@google.com

created issue #1

Apr 27, 2022 11:36PM

Macrobenchmarks needs to clear ART profiles between runs to ensure a clean slate before a measurement.

However, calling cmd package compile --reset is not supported on user builds at all. This means, that we can't actually rely on --reset.

Additional context: b/230518212

Comments

ng...@google.com <ng...@google.com> Apr 28, 2022 09:03AM

Reassigned to ma...@google.com.

ap...@google.com <ap...@google.com> #2Apr 28, 2022 10:22PM

Marked as fixed.

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...@google.com <ma...@google.com> #3Apr 29, 2022 10:00AM

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?

ra...@google.com <ra...@google.com> #4Apr 29, 2022 06:37PM

Status: Assigned (reopened)

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.

cc...@google.com <cc...@google.com> #5Apr 29, 2022 06:45PM

Marked as fixed, reassigned to ra...@google.com.

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

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

cc...@google.com <cc...@google.com> #6May 4, 2022 06:16PM

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

cc...@google.com <cc...@google.com> #7May 4, 2022 06:20PM

Filed b/231457688 to investigate this performance gap.

cc...@google.com <cc...@google.com> #8May 6, 2022 12:32AM

Note from perf gap investigation in #comment7 - it's coincidental, caused by shader compilation, tracked in b/231455742

sh...@gmail.com <sh...@gmail.com> #9Jul 15, 2022 11:21PM

In the latest stable version of the library (compared to beta verison), it looks we are reinstalling the apk during each iteration, which is clearing application data for each macrobenchmark. Issue with that approach is, when measuring the performance (esp. FrameTimingMetric) for screens deep inside, the onboarding flow of the app needs to be repeated every time which is not ideal. We just want to measure the performance improvements for users who are updating to the latest versions. Can we not just clear the clear ART profiles between runs. Can you please advice how this can be achieved. Do let me know if you need more details on this issue.

ra...@google.com <ra...@google.com> #10Jul 15, 2022 11:57PM

Unfortunately it's not possible to just clear ART profiles prior to Android T on user builds. This is why we need to re-install the APK.

On the next iteration of Macrobenchmark (1.2.0-alpha01), we will try and clear profile caches if the session is rooted.

sh...@gmail.com <sh...@gmail.com> #11Jul 28, 2022 03:17AM

Thanks for the quick and prompt response. Sorry which version of android are you referring here when you say Android T

be...@google.com <be...@google.com> #12Jul 28, 2022 08:21AM

Android T is the development name for Android 13.

th...@gmail.com <th...@gmail.com> #13May 19, 2023 03:06PM

Hi, coming from the android docs here (

https://developer.android.com/topic/performance/baselineprofiles/overview) and just wanted to be clear on a point.

"Resetting ART profile caches isn't allowed on user (non-rooted) builds. To work around this, androidx.benchmark:benchmark-macro-junit4:1.1.0 includes a fix that reinstalls the app during the benchmark (issue)."

When the docs say a "fix that reinstalls the app during the benchmark", and in the above comment, where you talk about 'clearing application data for each macrobenchmark', I just wanted to be clear this relates to running benchmarks only. It does not mean that when we upload our baseline profile with our APK version for the first time, our users will lose their application data? I'm new to this area of development so thanks for patience with the potentially rudimentary enquiry...!

ml...@google.com <ml...@google.com> #14May 19, 2023 03:24PM

The app data is cleared only when running the Macrobenchmark locally on the device as part of the verification/generation process. No action is done to either the published app, or to apps on other devices.

When you upload the baseline profile with the APK/AAB, it's already in the generated (binary) form and it won't clear any user data.