Skip to content

Commit 3b4abb3

Browse files
brownrice777meta-codesync[bot]
authored andcommitted
Make the view manager prop setter caches thread-safe (#59245)
Summary: `ViewManagersPropertyCache.CLASS_PROPS_CACHE` and the two setter maps in `ViewManagerPropertyUpdater` are static `HashMap`s. Two threads write them: - the UI thread, when it updates the props of a view (`ViewManagerPropertyUpdater.updateProps` → `findManagerSetter`); - the JS thread, when it builds a view config (`getConstantsForViewManager` → `ViewManager.nativeProps`). In bridgeless mode, JS gets the view config of a lazy view manager on first use, during render. At that time, the UI thread can mount other views. Concurrent writes to a `HashMap` can lose entries or break the map. This change uses `ConcurrentHashMap` for the three maps. The get-then-put code does not change. If two threads compute the same entry, the values are equal, so the second write is harmless. I did not use `computeIfAbsent`, because `getNativePropSettersForViewManagerClass` calls itself for the superclass. This came up in the review of #59173, which sends the eager view managers through the same on-demand path. ## Changelog: [ANDROID] [FIXED] - Make the view manager prop setter caches thread-safe Pull Request resolved: #59245 Test Plan: Unit tests: `./gradlew :packages:react-native:ReactAndroid:testDebugUnitTest --tests "com.facebook.react.uimanager.*"` passes (230 tests, 0 failures). Device: Xiaomi Mi Note 2 (Snapdragon 821, Android 8.0). App: a production Expo app, release build, React Native 0.88.0, Hermes. I compiled this change and #59173 against 0.88.0 and replaced the same classes in `react-android-0.88.0-release.aar`. I measured from process start to the first frame with rows in the first list, 12 cold starts for each build. | Build | Median | Range | `getConstants()` | |---|---|---|---| | Stock 0.88.0 | 2416 ms | 2335–2477 ms | 215 ms (45 keys) | | #59173 + this change | 2243 ms | 2187–2406 ms | 45 ms (5 keys) | To see the cost of this change alone, I also used a build whose JS does not call `getConstants()` at startup, so the first view configs come from `getConstantsForViewManager()` during render. Without this change: 2189 ms median (2140–2236 ms). With it: 2192 ms median (2156–2244 ms). I see no difference. Same chat app with threads list and messages. With this change, I opened the chat list, a thread, settings, the theme list, the profile and the contacts, and scrolled. Logcat shows no crash and no view config errors. Reviewed By: christophpurrer Differential Revision: D126631569 Pulled By: javache fbshipit-source-id: ae455c2a152f9dcf6e5bec172005db9e654e7193
1 parent 8f5a1c0 commit 3b4abb3

2 files changed

Lines changed: 9 additions & 3 deletions

File tree

‎packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManagerPropertyUpdater.kt‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import com.facebook.common.logging.FLog
1212
import com.facebook.react.bridge.ReadableArray
1313
import com.facebook.react.uimanager.ViewManagersPropertyCache.PropSetter
1414
import java.util.HashMap
15+
import java.util.concurrent.ConcurrentHashMap
1516

1617
@Suppress("DEPRECATION")
1718
public object ViewManagerPropertyUpdater {
@@ -31,8 +32,11 @@ public object ViewManagerPropertyUpdater {
3132

3233
private const val TAG = "ViewManagerPropertyUpdater"
3334

34-
private val VIEW_MANAGER_SETTER_MAP: MutableMap<Class<*>, ViewManagerSetter<*, *>> = HashMap()
35-
private val SHADOW_NODE_SETTER_MAP: MutableMap<Class<*>, ShadowNodeSetter<*>> = HashMap()
35+
// Used from the JS thread (view configs) and the UI thread (prop updates).
36+
private val VIEW_MANAGER_SETTER_MAP: MutableMap<Class<*>, ViewManagerSetter<*, *>> =
37+
ConcurrentHashMap()
38+
private val SHADOW_NODE_SETTER_MAP: MutableMap<Class<*>, ShadowNodeSetter<*>> =
39+
ConcurrentHashMap()
3640

3741
@JvmStatic
3842
public fun clear() {

‎packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManagersPropertyCache.kt‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import com.facebook.react.uimanager.annotations.ReactProp
2121
import com.facebook.react.uimanager.annotations.ReactPropGroup
2222
import com.facebook.yoga.YogaValue
2323
import java.lang.reflect.Method
24+
import java.util.concurrent.ConcurrentHashMap
2425

2526
/**
2627
* This class is responsible for holding view manager property setters and is used in a process of
@@ -29,7 +30,8 @@ import java.lang.reflect.Method
2930
@Suppress("DEPRECATION") // ReactShadowNode is @Deprecated but still required by this cache.
3031
internal object ViewManagersPropertyCache {
3132

32-
private val CLASS_PROPS_CACHE: MutableMap<Class<*>, Map<String, PropSetter>> = HashMap()
33+
// Used from the JS thread (view configs) and the UI thread (prop updates).
34+
private val CLASS_PROPS_CACHE: MutableMap<Class<*>, Map<String, PropSetter>> = ConcurrentHashMap()
3335
private val EMPTY_PROPS_MAP: MutableMap<String, PropSetter> = HashMap()
3436

3537
@JvmStatic

0 commit comments

Comments
 (0)