Scaffold innerPadding can cause tons of recompositions of the whole content lambda [373904168]

Fixed

Bug

Status Update

No update yet.

Description

ml...@google.com

created issue #1

Oct 16, 2024 07:21PM

Adding a scrollBehavior to a TopAppBar causes the innerPadding property to recompose with the layout changes of the appbar. The innerPadding propery is then passed into the content lambda, which is read in composition and therefore recomposing the content on every frame causing huge performance issue given Scaffold is used usually per-screen.

Deferring reading the innerPadding in the userland code doesn't fix the issue.

Reproducible snippet:

@OptIn(ExperimentalMaterial3Api::class)
@Composable
fun InnerPaddingRecomposingWholeContent() {
    val scrollBehavior = TopAppBarDefaults.enterAlwaysScrollBehavior()

    Scaffold(
        topBar = {
            TopAppBar(
                colors = TopAppBarDefaults.topAppBarColors(containerColor = MaterialTheme.colorScheme.primary),
                title = { Text("hello") },
                scrollBehavior = scrollBehavior
            )
        },
        modifier = Modifier
            .nestedScroll(scrollBehavior.nestedScrollConnection)
    ) { innerPadding ->
        // Here the innerPadding causes huge number of recompositions of the whole lambda with LazyColumn.
        // This causes extreme jank in cases of somewhat complex screen (reported by X).
        LazyColumn(
            modifier = Modifier
                // Our standard recommended solution
                .padding(innerPadding)
                // deferring the innerPadding this way won't work, because the innerPadding is read in composition and passed to the lambda as a value
                // .offset { IntOffset(0, innerPadding.calculateTopPadding().roundToPx()) }
        ) {
            items(100) { index ->
                ListItem(headlineContent = { Text(text = "Hello $index!") })
            }
        }
    }
}

This behavior also affects material.Scaffold, which can be fixed as well.

Comments

ap...@google.com <ap...@google.com> #2Oct 17, 2024 06:04PM

Marked as fixed.

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

ml...@google.com <ml...@google.com> #3Oct 17, 2024 07:24PM

Did some benchmarks in Jetcaster when slow scrolling. 32 less compositions.

ScrollBenchmark_fixedScaffold
Scaffold.contentCount          min   2.0,   median   2.0,   max   3.0
Scaffold.contentSumMs          min   0.5,   median   0.9,   max   1.1
frameCount                     min 198.0,   median 198.0,   max 498.0
frameDurationCpuMs             P50    4.9,   P90    6.6,   P95    8.7,   P99   24.1
frameOverrunMs                 P50   -7.5,   P90   -3.4,   P95    0.6,   P99   21.7

ScrollBenchmark_normalScaffold
Scaffold.contentCount          min  34.0,   median  37.0,   max  37.0
Scaffold.contentSumMs          min  16.7,   median  17.5,   max  17.7
frameCount                     min 219.0,   median 225.0,   max 231.0
frameDurationCpuMs             P50    5.0,   P90    6.9,   P95    9.1,   P99   27.0
frameOverrunMs                 P50   -6.6,   P90   -3.1,   P95    0.1,   P99   18.8

pr...@google.com <pr...@google.com> #4Oct 30, 2024 05:37PM

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

androidx.compose.material:material:1.8.0-alpha05
androidx.compose.material:material-android:1.8.0-alpha05
androidx.compose.material:material-jvmstubs:1.8.0-alpha05
androidx.compose.material:material-linuxx64stubs:1.8.0-alpha05
androidx.compose.material3:material3:1.4.0-alpha03
androidx.compose.material3:material3-android:1.4.0-alpha03
androidx.compose.material3:material3-jvmstubs:1.4.0-alpha03
androidx.compose.material3:material3-linuxx64stubs:1.4.0-alpha03

Issue 373904168

Description

Issue summary

Comments

ap...@google.com <ap...@google.com> #2Oct 17, 2024 06:04PM

ml...@google.com <ml...@google.com> #3Oct 17, 2024 07:24PM

pr...@google.com <pr...@google.com> #4Oct 30, 2024 05:37PM

Add comment

Issue metadata