Skip to content

Commit 96c9247

Browse files
authored
Skip the unchanged scroll state update when a ScrollView mounts on Android
## Summary: On Android, every `ScrollView` commits one state update on mount that changes nothing. `onLayout` calls `scrollTo`, which calls `ReactScrollViewHelper.updateFabricScrollState`. That method dedupes against `lastStateUpdateScroll`, but this starts at `(-1, -1)`, so the first offset always gets through, even when it's the `(0, 0)` already in the state. The update then runs a full commit on the JS thread (clone, layout and differ) and sends an `UpdateState` item back to Java. On iOS, `_updateStateWithContentOffset` already skips an update when the offset hasn't changed. Now, before a view's first update, `updateFabricScrollState` reads the offset from the state into `lastStateUpdateScroll`, and the existing dedupe does the rest. This costs one state read per view, not one per scroll frame. I didn't start `lastStateUpdateScroll` at `(0, 0)`, because a view created for a state that already holds another offset would then skip the update that corrects the state. ## Changelog: [ANDROID] [CHANGED] - `ScrollView` no longer commits an unchanged scroll state when it mounts ## Test Plan: Unit tests: the new `ReactScrollViewHelperStateTest` covers a first update that matches the state (not sent), one that differs from it (sent), one without state data (sent), and later updates (sent once per offset, with one state read). The two tests that expect a skipped update fail without this change. ``` ./gradlew :packages:react-native:ReactAndroid:testDebugUnitTest --tests 'com.facebook.react.views.scroll.*' ``` 25 tests, 0 failures. `yarn format-kotlin` makes no further changes. Behavior: on a Galaxy A16, a temporary Playground screen read a marker's position with `measureInWindow`, and both builds reported the same positions at every step: - after mount with `contentOffset`, vertical and horizontal - after `scrollTo`, vertical and horizontal - after scrolling back to 0 - after toggling `display: none` on the parent Measurements on a Galaxy A16 (Android 16): two release RNTester APKs built from this commit's base, one with and one without this change, with default feature flags, compiled with `cmd package compile -m speed`. A temporary Playground screen mounts and unmounts the scenario 12 times. JS thread CPU comes from `/proc/<pid>/task/<tid>/stat`. The APKs were installed in alternating blocks, A/B/B/A twice, which gives 4 fresh processes per arm for each scenario. The table shows medians per mount and unmount: | Scenario | Without this change | With this change | Change | |---|--:|--:|--:| | 100 `ScrollView`s, per `ScrollView` | 1050 µs | 554 µs | −47% | | Vertical `ScrollView` with 5 horizontal ones and 40 cards | 86.6 ms | 82.5 ms | −4.8% | | One `ScrollView` with 300 cards | 307.0 ms | 306.2 ms | no change | | `FlatList` with 40 cards | 92.0 ms | 92.9 ms | no change | That's about 0.5–0.7 ms of JS time per `ScrollView` on this phone, so it only shows on screens with several of them. The 100-`ScrollView` case is artificial and shows the cost of each commit. In the second scenario the ranges overlap slightly (85.0–88.3 ms and 79.2–85.8 ms). UI thread time didn't change in any scenario. Not tested on a device: `enableViewRecyclingForScrollView` and `enableViewCulling`. The `display: none` toggle may not recreate the view, so the case of a state that already holds another offset is covered only by the unit test. With `enableViewCulling` or `useTraitHiddenOnAndroid` on, `loadFabricScrollState` already sets `lastStateUpdateScroll`, so the new read doesn't run after that. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Differential Revision: D129045292 Pull Request resolved: #59719
1 parent 3b3fce3 commit 96c9247

2 files changed

Lines changed: 132 additions & 1 deletion

File tree

‎packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollViewHelper.kt‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ public object ReactScrollViewHelper {
4040
private val TAG = ReactScrollView::class.java.simpleName
4141
private val DEBUG_MODE = false // ReactBuildConfig.DEBUG
4242

43+
private const val UNSET_SCROLL_POSITION = -1
4344
private const val CONTENT_OFFSET_LEFT = "contentOffsetLeft"
4445
private const val CONTENT_OFFSET_TOP = "contentOffsetTop"
4546
private const val SCROLL_AWAY_PADDING_TOP = "scrollAwayPaddingTop"
@@ -373,6 +374,17 @@ public object ReactScrollViewHelper {
373374
// User driven scrolling should disable scroll state updates coming from Fabric
374375
scrollState.isUpdatedByScroll = true
375376

377+
// Until a view sends its first update it doesn't know the offset in its state. On mount that
378+
// offset is usually already correct, and sending it again would only commit an unchanged tree.
379+
if (scrollState.lastStateUpdateScroll.equals(UNSET_SCROLL_POSITION, UNSET_SCROLL_POSITION)) {
380+
scrollView.stateWrapper?.stateData?.let { stateData ->
381+
scrollState.setLastStateUpdateScroll(
382+
toPixelFromDIP(stateData.getDouble(CONTENT_OFFSET_LEFT)).toInt(),
383+
toPixelFromDIP(stateData.getDouble(CONTENT_OFFSET_TOP)).toInt(),
384+
)
385+
}
386+
}
387+
376388
// Dedupe events to reduce JNI traffic
377389
if (scrollState.lastStateUpdateScroll.equals(scrollX, scrollY)) {
378390
return
@@ -660,7 +672,7 @@ public object ReactScrollViewHelper {
660672
/** Get the padding on the bottom for tab bar */
661673
var scrollAwayPaddingBottom: Int = 0,
662674
/** Get the Fabric state of last scroll position */
663-
val lastStateUpdateScroll: Point = Point(-1, -1),
675+
val lastStateUpdateScroll: Point = Point(UNSET_SCROLL_POSITION, UNSET_SCROLL_POSITION),
664676
/** Get true if the previous animation was canceled */
665677
var isCanceled: Boolean = false,
666678
/** Get true if previous animation was finished */
Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
/*
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*/
7+
8+
package com.facebook.react.views.scroll
9+
10+
import android.annotation.SuppressLint
11+
import android.util.DisplayMetrics
12+
import com.facebook.react.bridge.ReactApplicationContext
13+
import com.facebook.react.bridge.ReactTestHelper.createTestReactApplicationContext
14+
import com.facebook.react.bridge.ReadableNativeMap
15+
import com.facebook.react.internal.featureflags.ReactNativeFeatureFlagsForTests
16+
import com.facebook.react.uimanager.DisplayMetricsHolder
17+
import com.facebook.react.uimanager.StateWrapper
18+
import com.facebook.react.uimanager.ThemedReactContext
19+
import com.facebook.testutils.shadows.ShadowNativeLoader
20+
import com.facebook.testutils.shadows.ShadowNativeMap
21+
import com.facebook.testutils.shadows.ShadowReadableNativeMap
22+
import com.facebook.testutils.shadows.ShadowSoLoader
23+
import com.facebook.testutils.shadows.ShadowWritableNativeMap
24+
import org.junit.After
25+
import org.junit.Before
26+
import org.junit.Test
27+
import org.junit.runner.RunWith
28+
import org.mockito.kotlin.any
29+
import org.mockito.kotlin.doReturn
30+
import org.mockito.kotlin.mock
31+
import org.mockito.kotlin.never
32+
import org.mockito.kotlin.times
33+
import org.mockito.kotlin.verify
34+
import org.robolectric.RobolectricTestRunner
35+
import org.robolectric.annotation.Config
36+
37+
/** Verifies when a ScrollView sends its scroll offset to the Fabric state. */
38+
@SuppressLint("DeprecatedClass", "DeprecatedMethod")
39+
@RunWith(RobolectricTestRunner::class)
40+
@Config(
41+
shadows =
42+
[
43+
ShadowNativeLoader::class,
44+
ShadowSoLoader::class,
45+
ShadowNativeMap::class,
46+
ShadowReadableNativeMap::class,
47+
ShadowWritableNativeMap::class,
48+
],
49+
)
50+
class ReactScrollViewHelperStateTest {
51+
52+
private lateinit var view: ReactScrollView
53+
54+
@Before
55+
fun setup() {
56+
ReactNativeFeatureFlagsForTests.setUp()
57+
val context: ReactApplicationContext = createTestReactApplicationContext()
58+
DisplayMetricsHolder.setScreenDisplayMetrics(DisplayMetrics().apply { density = 1f })
59+
view =
60+
ReactScrollViewManager().createViewInstance(ThemedReactContext(context, context, null, -1))
61+
}
62+
63+
@After
64+
fun teardown() {
65+
DisplayMetricsHolder.setScreenDisplayMetrics(null)
66+
}
67+
68+
private fun stateWrapperWithOffset(x: Double, y: Double): StateWrapper {
69+
val stateData =
70+
mock<ReadableNativeMap> {
71+
on { getDouble("contentOffsetLeft") } doReturn x
72+
on { getDouble("contentOffsetTop") } doReturn y
73+
}
74+
return mock { on { this.stateData } doReturn stateData }
75+
}
76+
77+
@Test
78+
fun testFirstUpdateMatchingTheStateIsNotSent() {
79+
val stateWrapper = stateWrapperWithOffset(0.0, 0.0)
80+
view.stateWrapper = stateWrapper
81+
82+
ReactScrollViewHelper.updateFabricScrollState(view, 0, 0)
83+
84+
verify(stateWrapper, never()).updateState(any())
85+
}
86+
87+
@Test
88+
fun testFirstUpdateDifferingFromTheStateIsSent() {
89+
val stateWrapper = stateWrapperWithOffset(0.0, 500.0)
90+
view.stateWrapper = stateWrapper
91+
92+
ReactScrollViewHelper.updateFabricScrollState(view, 0, 0)
93+
94+
verify(stateWrapper, times(1)).updateState(any())
95+
}
96+
97+
@Test
98+
fun testFirstUpdateIsSentWithoutStateData() {
99+
val stateWrapper = mock<StateWrapper> { on { stateData } doReturn null }
100+
view.stateWrapper = stateWrapper
101+
102+
ReactScrollViewHelper.updateFabricScrollState(view, 0, 0)
103+
104+
verify(stateWrapper, times(1)).updateState(any())
105+
}
106+
107+
@Test
108+
fun testLaterUpdatesAreSentOncePerOffset() {
109+
val stateWrapper = stateWrapperWithOffset(0.0, 0.0)
110+
view.stateWrapper = stateWrapper
111+
112+
ReactScrollViewHelper.updateFabricScrollState(view, 0, 0)
113+
ReactScrollViewHelper.updateFabricScrollState(view, 0, 100)
114+
ReactScrollViewHelper.updateFabricScrollState(view, 0, 100)
115+
116+
verify(stateWrapper, times(1)).updateState(any())
117+
verify(stateWrapper, times(1)).stateData
118+
}
119+
}

0 commit comments

Comments
 (0)