Cold startup measurements inconsistent due to shader compilation [231455742]

Fixed

Bug

Status Update

No update yet.

Description

cc...@google.com

created issue #1

May 4, 2022 06:04PM

Alert on dashboard: https://androidx-perf.skia.org/t/?begin=1651608277&end=1651608278&subset=all

CLs in build: https://android-build.googleplex.com/builds/jump-to-build/8534762

Caused by https://android-review.googlesource.com/c/platform/frameworks/support/+/2086023/ - Improve macrobench iter speed by optimizing shell commands

Regression only affects baseline profile and full compilation variants of cold startup benchmarks.

Need to see if this makes the numbers more or less correct, and why. Triaging to 1.1 for this reason.

Comments

cc...@google.com <cc...@google.com> #2May 5, 2022 10:58PM

Reassigned to dj...@google.com.

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

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

Thu May 05 2022 15:37:01 GMT-0700 (Pacific Daylight Time).png

30 KB

View

Download

Thu May 05 2022 15:40:17 GMT-0700 (Pacific Daylight Time).png

30 KB

View

Download

cc...@google.com <cc...@google.com> #3May 6, 2022 12:30AM

Dupe bug is another instance of same problem, but hitting different benchmarks. See alert word cloud for this bug, dupe bug.

ra...@google.com <ra...@google.com> May 24, 2022 08:24PM

Reassigned to ra...@google.com.

ra...@google.com <ra...@google.com> #4May 24, 2022 10:44PM

Marked as fixed.

Fixed by https://android-review.googlesource.com/c/platform/frameworks/support/+/2098079.

cc...@google.com <cc...@google.com> Oct 18, 2022 08:23PM

Status: Assigned (reopened)

cc...@google.com <cc...@google.com> #5Oct 18, 2022 09:39PM

The current dropShaderCache implementation doesn't work on root or user builds.

I added a trace section metric for cache_miss/cache_hit (traced by Skia for shader compilation), and see that on rooted devices, without uninstall/reinstall, the shader gets compiled and left in place:

## Compilation reset with pm compile --reset

SmallListStartupBenchmark_startup[startup=COLD,compilation=None]
           cache_hitMs   min   1.4,   median   1.4,   max   1.5
          cache_missMs   min   0.0,   median   0.0,   max   0.0
timeToInitialDisplayMs   min 240.0,   median 255.4,   max 273.7
Traces: Iteration 0 1 2 3 4 5 6 7 8 9
SmallListStartupBenchmark_startup[startup=COLD,compilation=WarmupProfile(iterations=3)]
           cache_hitMs   min   1.3,   median   1.4,   max   3.0
          cache_missMs   min   0.0,   median   0.0,   max   0.0
timeToInitialDisplayMs   min 190.6,   median 202.7,   max 211.4
Traces: Iteration 0 1 2 3 4 5 6 7 8 9
SmallListStartupBenchmark_startup[startup=COLD,compilation=Full]
           cache_hitMs   min   1.4,   median   1.4,   max   3.1
          cache_missMs   min   0.0,   median   0.0,   max   0.0
timeToInitialDisplayMs   min 222.8,   median 232.5,   max 257.5
Traces: Iteration 0 1 2 3 4 5 6 7 8 9

While when the reinstall is used, we sometimes hit shader compilation (cache_miss), and sometimes don't, which is affected by how long the app stays alive.

## Compilation reset with uninstall + reinstall

SmallListStartupBenchmark_startup[startup=COLD,compilation=None]
           cache_hitMs   min   0.0,   median   0.0,   max   0.0
          cache_missMs   min  22.1,   median  22.6,   max  24.0
timeToInitialDisplayMs   min 258.6,   median 264.6,   max 276.4
Traces: Iteration 0 1 2 3 4 5 6 7 8 9
SmallListStartupBenchmark_startup[startup=COLD,compilation=WarmupProfile(iterations=3)]
           cache_hitMs   min   1.4,   median   1.4,   max   1.4
          cache_missMs   min   0.0,   median   0.0,   max   0.0
timeToInitialDisplayMs   min 191.4,   median 195.9,   max 214.7
Traces: Iteration 0 1 2 3 4 5 6 7 8 9
SmallListStartupBenchmark_startup[startup=COLD,compilation=Full]
           cache_hitMs   min   0.0,   median   0.0,   max   0.0
          cache_missMs   min  22.3,   median  23.3,   max  23.8
timeToInitialDisplayMs   min 232.2,   median 245.7,   max 262.9
Traces: Iteration 0 1 2 3 4 5 6 7 8 9

In trivially quick cold startup tests, where the process is killed quickly, we see the shaders never get a chance to compile. With warmup profiles, where the app is alive for a while (and they have time to be written to disk) they do.

This currently makes warmup iterations look better than they should.

Only fix I have figured out on user builds is broadcasting to the app to delete shaders, since on user builds shell doesn't have access to the shader cache dir.

ap...@google.com <ap...@google.com> #6Oct 20, 2022 05:31PM

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

commit d5fb7b68ab904d904c21cbb8ce0bf497979ff6a1
Author: Chris Craik <ccraik@google.com>
Date: Tue Oct 18 14:16:36 2022

Add summing mode to TraceSectionMetric

Test: TraceSectionMetricTest
Relnote: """
Added TraceSectionMode("label", Mode.Sum), allowing measurement of
total time spent on multiple trace sections with the same label. For
instance, TraceSectionMetric("inflate", Mode.Sum) will report a metric
`inflateMs` for the total time in a trace spent on inflation.

Also removed API requirement, as TraceSectionMetric works together
with androidx.tracing.Trace back to lower API levels, with the use of
[forceEnableAppTracing](

https://developer.android.com/reference/androidx/tracing/Trace#forceEnableAppTracing()).
"""

Additionally adds tracing of cache_miss and cache_hit for observing
cost of shader compilation for

b/231455742

Change-Id: Id7b68e23f5ded4d20ab21771dbf9eb96d9dcfdb7

M benchmark/benchmark-macro/api/public_plus_experimental_current.txt
M benchmark/benchmark-macro/src/androidTest/java/androidx/benchmark/macro/TraceSectionMetricTest.kt
M benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/Metric.kt
M compose/integration-tests/macrobenchmark/src/androidTest/java/androidx/compose/integration/macrobenchmark/SmallListStartupBenchmark.kt
M testutils/testutils-macrobenchmark/src/main/java/androidx/testutils/MacrobenchUtils.kt

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

ap...@google.com <ap...@google.com> #7Oct 27, 2022 03:53PM

Marked as fixed.

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

commit 0ba5c3dad9652a85319894f9aa7ad52664be72d2
Author: Chris Craik <ccraik@google.com>
Date: Tue Oct 18 20:56:23 2022

Fix deleteShaderCache by using Profileinstaller or fallback correctly with root

Test: SmallListStartupBenchmark
Test: MacrobenchmarkScopeTest
Fixes: 231455742

Relnote: """
BENCHMARK RELEASE NOTES
Fixed MacrobenchmarkScope.dropShaderCache(). This removes roughly 20ms
of noise from StartupMode.COLD benchmarks, as shaders are now
consistently cleared each iteration. Previously, `Partial` compilation
using warmup iterations would report incorrectly fast numbers, as
shader caching was more likely to happen during warmup. This fix
requires either a rooted device, or using
`profileinstaller:1.3.0-alpha02` in the target app.

PROFILEINSTALLER RELEASE NOTES
Added a hook for benchmarks to drop the shader cache, to ensure
consistent performance for cold startups, especially when compiling
with profiles from warmup iterations. This update is required to
measure cold startups using `benchmark-macro-junit4:1.2.0-alpha05` or
later.
"""

Change-Id: Ia5171b0f40dd8ce6f64f5ccf0a33281a4d8b121e

M benchmark/benchmark-macro/src/androidTest/java/androidx/benchmark/macro/MacrobenchmarkScopeTest.kt
M benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/Macrobenchmark.kt
M benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/MacrobenchmarkScope.kt
M benchmark/benchmark-macro/src/main/java/androidx/benchmark/macro/ProfileInstallBroadcast.kt
M profileinstaller/profileinstaller/api/current.txt
M profileinstaller/profileinstaller/api/public_plus_experimental_current.txt
M profileinstaller/profileinstaller/api/restricted_current.txt
A profileinstaller/profileinstaller/src/main/java/androidx/profileinstaller/BenchmarkOperation.java
M profileinstaller/profileinstaller/src/main/java/androidx/profileinstaller/ProfileInstallReceiver.java
M profileinstaller/profileinstaller/src/main/java/androidx/profileinstaller/ProfileInstaller.java

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

na...@google.com <na...@google.com> #8Nov 9, 2022 06:29PM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.benchmark:benchmark-macro:1.2.0-alpha07
androidx.profileinstaller:profileinstaller:1.3.0-alpha02