Benchmarks - Regression found in "Bump compose.material3 to 1.3.0-alpha01 and unpin dependencies" [320565175]

Fixed

Bug

Status Update

No update yet.

Description

sk...@skia-public.iam.gserviceaccount.com

created issue on behalf of an...@google.com #1

Jan 20, 2024 03:56AM

Perf Regression (High) found, matching 2 tracked metrics from benchmarks.

To triage this regression, see the guide at go/androidx-bench-triage.

Tests affected:

 androidx.compose.material3.benchmark.TabRowBenchmark_selectTab

Devices affected:

 sargo

API Level:

 API_30_R
 API_31_S

Comments

gr...@google.com <gr...@google.com> #2Jan 22, 2024 11:56PM

Reassigned to co...@google.com.

Looks like this use case in particular may have a significant regression, possibly due to the unpinning of dependencies in aosp/2918471.

Going to assign to Connie since she is on the compose side of material from that CL.

co...@google.com <co...@google.com> Jan 23, 2024 12:01AM

Reassigned to se...@google.com.

se...@google.com <se...@google.com> Jan 23, 2024 05:28PM

Accepted by se...@google.com.

se...@google.com <se...@google.com> #3Jan 24, 2024 07:14PM

While I am currently investigating, I believe this is being caused by a "regression" in selectable from foundation. I have regression in quotations because in updating foundation from 1.6.0-rc01 to project(":compose:foundation:foundation"), I believe our library is now pointing to 1.5.4, the last stable release of the foundation library. Observe the following local results:

test	changes applied	nanoseconds	allocs
EMULATOR_TabRowBenchmark.selectTab	Checkout aosp/2918471 and revert builds	107,885	477
EMULATOR_TabRowBenchmark.selectTab	Checkout aosp/2918471 and revert builds	99,803	477
EMULATOR_TabRowBenchmark.selectTab	Checkout aosp/2918471 and revert builds	109,119	477
EMULATOR_TabRowBenchmark.selectTab	Checkout aosp/2918471 and revert builds	105,712	477
EMULATOR_TabRowBenchmark.selectTab	Checkout aosp/2918471 and revert builds	103,714	477
EMULATOR_TabRowBenchmark.selectTab	foundation:1.6.0-rc01, remove selectable	94,252	386
EMULATOR_TabRowBenchmark.selectTab	foundation:1.6.0-rc01 remove selectable	89,707	386
EMULATOR_TabRowBenchmark.selectTab	foundation:1.6.0-rc01 remove selectable	82,975	386
EMULATOR_TabRowBenchmark.selectTab	foundation:1.6.0-rc01 remove selectable	84,898	386
EMULATOR_TabRowBenchmark.selectTab	foundation:1.6.0-rc01 remove selectable	85,916	386
EMULATOR_TabRowBenchmark.selectTab	Update foundation to project call	242,971	1304
EMULATOR_TabRowBenchmark.selectTab	Update foundation to project call	245,222	1304
EMULATOR_TabRowBenchmark.selectTab	Update foundation to project call	236,542	1304
EMULATOR_TabRowBenchmark.selectTab	Update foundation to project call	251,981	1304
EMULATOR_TabRowBenchmark.selectTab	Update foundation to project call	238,440	1304
EMULATOR_TabRowBenchmark.selectTab	Updated foundation: remove selectable	83,522	386
EMULATOR_TabRowBenchmark.selectTab	Updated foundation: remove selectable	88,514	386
EMULATOR_TabRowBenchmark.selectTab	Updated foundation: remove selectable	87,114	386
EMULATOR_TabRowBenchmark.selectTab	Updated foundation: remove selectable	86,855	386
EMULATOR_TabRowBenchmark.selectTab	Updated foundation: remove selectable	85,822	386

This update appears to increase the allocations caused by selectable from ~91 to ~918 and an avg runtime increase of ~17k ns to ~156k ns. Granted these are emulator results, the change is very significant.

se...@google.com <se...@google.com> #4Jan 24, 2024 10:55PM

Correction, unpinning points our dependencies to tip of tree.

Pinning our dependencies to 1.6.0-rc01 at aosp/2918471, and as late as the following ripple change (aosp/2845575) fixes this regression.

However, pinning our dependencies at ToT does not fix this regression. Therefor, it is highly possible that a change at ripple or after, combined with updating our dependencies to ToT causes this regression.

We should investigate if there is a change that has landed in foundation after the 1.6.0-rc01 cut which combined with ripple updates causes this regression to occur. CC: Louis

lp...@google.com <lp...@google.com> #5Jan 24, 2024 11:46PM

>Pinning our dependencies to 1.6.0-rc01 at aosp/2918471, and as late as the following ripple change (aosp/2845575) fixes this regression.

>However, pinning our dependencies at ToT does not fix this regression. Therefor, it is highly possible that a change at ripple or after, combined with updating our dependencies to ToT causes this regression.

Sorry, not sure I understand here - are you saying that when you unpin dependencies, at ToT (with the ripple change), the issue occurs? Currently this specific case (changing parameters to selectable / clickable / etc) is known to be a bit slower, this should be fixed by the next alpha / the one after, but I am surprised it is to the magnitude you point out. It would be good to test on a real device if possible, since emulators are not reliable, and also if you could test with this CL reverted:

http://aosp/2889886

That would help inform as to whether this case I mentioned is to blame here

lp...@google.com <lp...@google.com> #6Jan 24, 2024 11:48PM

Also, were there any other benchmarks affected?

lp...@google.com <lp...@google.com> #7Jan 24, 2024 11:54PM

Also, do you have numbers for the firstPixel benchmark for tabs too? I would expect that to have improved here

se...@google.com <se...@google.com> #8Jan 25, 2024 06:42PM

The regression was caused by unpinning the dependencies in our library on 1/19. Additionally, as of the ripple update on 1/22, re-pinning the dependencies no longer resolves the issue (note I cannot pin ripple here because the new changes depend on features at tip of tree). My hunch is that there is an additional dependency in the ripple library that perhaps overrides one of ours. Still continuing to investigate, and I will try and verify these on a physical device as well. firstpixel was not affected by the regression, and has seen slight improvement. It's specifically the state toggle test.

I dont think other components have seen a regression. The closest component is NavigationBar which uses selectable, selectablegroup, however does not use ripple.

lp...@google.com <lp...@google.com> #9Jan 30, 2024 01:46PM

Assigned to lp...@google.com.

Turns out this is a (known) regression because of the clickable / selectable refactoring / rewriting. I'll prioritize fixing this regression while we make changes to other parts of the system

ap...@google.com <ap...@google.com> #10Feb 1, 2024 02:22PM

Marked as fixed.

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

commit 176632f8e826524658d782b03c9d27892d047819
Author: Louis Pullen-Freilich <lpf@google.com>
Date: Wed Jan 31 15:59:06 2024

Turns off auto invalidation in clickable / focusable

This causes a lot of extra work when input parameters are changed, such as invalidating focus nodes - even though most of these parameters do not affect the state of the node tree.

Bug:

b/298048146

Fixes:

b/320565175

Test: ClickableTest
Test: TabRowBenchmark
Change-Id: I2fcc8732784d27fde072ac739f917f6da5a432a6

M compose/foundation/foundation/src/androidInstrumentedTest/kotlin/androidx/compose/foundation/ClickableTest.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/Clickable.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/Focusable.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/FocusedBounds.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/relocation/BringIntoViewRequester.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/relocation/BringIntoViewResponder.kt

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

pr...@google.com <pr...@google.com> #11Feb 21, 2024 07:33PM

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

androidx.compose.foundation:foundation:1.7.0-alpha03
androidx.compose.foundation:foundation-android:1.7.0-alpha03
androidx.compose.foundation:foundation-desktop:1.7.0-alpha03