Benchmarks file many bugs for changes unrelated to the benchmark [354264743]

Assigned

Bug

Status Update

No update yet.

Description

mo...@google.com

created issue #1

Jul 19, 2024 09:47PM

I am currently the triage cop for toolkit and see many bugs created for false positives on benchmarks (both high and low). The symptom is that an unrelated change affects a benchmark. They usually bounce between two different values, but not in a noisy way. The metric will show high for a while, then low for a while, then high again. A bug is filed for both the high and low changes.

Comments

ap...@google.com <ap...@google.com> #2Jul 19, 2024 09:52PM

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

mo...@google.com <mo...@google.com> #3Jul 22, 2024 04:16PM

This CL doesn't seem to have fixed all the problems. I see this bug: b/354309611 that showed up recently. There is something completely outside of the scope of the tests affecting the metric.

ap...@google.com <ap...@google.com> #4Jul 31, 2024 08:57PM

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

commit b3cff5328dfc448de4a05b4722806ea8b7ce33b9
Author: Chris Craik <ccraik@google.com>
Date: Tue Jul 30 15:48:16 2024

Reland Experiment: Enable R8 in foundation-library and compose-runtime benchmarks

Bug: 354264743
Test: DbMonsterBenchmark#dbMonster_count10_mutate10 (D8 = 1,677,855, R8 = 1,266,340)

Benchmarks appear to bounce around when something in the library
changes, regardless of whether it should affect the benchmark.
For example, adding a new collection could cause a benchmark
to increase or decrease, even though the collection hasn't
been used anywhere.

This CL set minifyEnabled to true for compose foundation-library
and compose runtime benchmarks, two libraries that have seen
recent performance variability. This should be considered an
experiment that may be removed in the future or expanded to
other libraries, depending on whether this helps performance
alerts.

(Reland with correct androidTest property)

Test: ran perf tests locally
Change-Id: I0740dd5fd83dd87e7bad95bbfaf2950593a1ec3a

M compose/foundation/foundation-layout/benchmark/build.gradle
M compose/runtime/runtime/compose-runtime-benchmark/build.gradle

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

cc...@google.com <cc...@google.com> #5Aug 16, 2024 08:34PM

Note this had to be reverted again here: https://android-review.git.corp.google.com/c/platform/frameworks/support/+/3205832 due to failures to load listeners set by instrumentation arg.

ap...@google.com <ap...@google.com> #6Aug 28, 2024 08:28PM

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

commit ef02fd556a27c5ff582c4363bf39ec23faf34541
Author: Chris Craik <ccraik@google.com>
Date: Mon Aug 26 16:06:43 2024

Add JUnit runlisteners to benchmark proguard

Bug: 354264743
Test: ./gradlew compose:foundation:foundation-benchmark:cC -P android.testInstrumentationRunnerArguments.class=androidx.compose.foundation.benchmark.NestedScrollerBenchmark -P android.testInstrumentationRunnerArguments.listener=androidx.benchmark.junit4.SideEffectRunListener # with R8 enabled in the module

Relnote: "Keep subclasses of org.junit.runner.notification.RunListener
in benchmark library proguard"

Previously we covered InstrumentationRunListeners (such as we use for
reporting json files) in the proguard file, but were missing the JUnit ones.

Change-Id: Ic8ed5d229c53d4f7779429e003331c251a5f1b39

M benchmark/benchmark-junit4/proguard-rules.pro

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

Issue 354264743