From 7d6d5daa2b8622c671ce279774f915dd9b4543c2 Mon Sep 17 00:00:00 2001 From: David Vacca Date: Tue, 1 Sep 2020 17:06:43 -0700 Subject: [PATCH] Refactor caching of Spannable objects instide TextLayoutManager Summary: This diff optimizes the caching of Spannable objects managed by the TextLayoutManager class. Previously, these objects were cached using unsing a String representation of the RedableMap (creating this string adds a non trivial cost), this diff improves the caching performance relying on the equals / hashcode methods of the ReadableNativeMap class I created a MC just to have a killswitch Motivation: I was analysing another bug and I found this non performant code changelog: [internal] internal Reviewed By: shergin Differential Revision: D23429365 fbshipit-source-id: 59e5ad0b1b95da992ac393aecfe029da68a8df97 --- .../react/config/ReactFeatureFlags.java | 3 ++ .../java/com/facebook/react/views/text/BUCK | 1 + .../react/views/text/TextLayoutManager.java | 40 +++++++++++++++---- 3 files changed, 36 insertions(+), 8 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java index 7a6b8a95ae4..6c2bd52a80f 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java +++ b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java @@ -71,4 +71,7 @@ public class ReactFeatureFlags { /** Use experimental SetState retry mechanism in view? */ public static boolean enableExperimentalStateUpdateRetry = false; + + /** Enable caching of Spannable objects using equality of ReadableNativeMaps */ + public static boolean enableSpannableCacheByReadableNativeMapEquality = true; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/BUCK b/ReactAndroid/src/main/java/com/facebook/react/views/text/BUCK index cd966af0699..ac69e03c258 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/BUCK +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/BUCK @@ -22,6 +22,7 @@ rn_android_library( react_native_dep("third-party/java/jsr-305:jsr-305"), react_native_target("java/com/facebook/react/bridge:bridge"), react_native_target("java/com/facebook/react/common:common"), + react_native_target("java/com/facebook/react/config:config"), react_native_target("java/com/facebook/react/module/annotations:annotations"), react_native_target("java/com/facebook/react/uimanager:uimanager"), react_native_target("java/com/facebook/react/uimanager/annotations:annotations"), diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java index ce32acc099d..b9ec0233d8a 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java @@ -26,6 +26,8 @@ import androidx.annotation.Nullable; import com.facebook.common.logging.FLog; import com.facebook.react.bridge.ReadableArray; import com.facebook.react.bridge.ReadableMap; +import com.facebook.react.bridge.ReadableNativeMap; +import com.facebook.react.config.ReactFeatureFlags; import com.facebook.react.uimanager.PixelUtil; import com.facebook.react.uimanager.ReactStylesDiffMap; import com.facebook.react.uimanager.ViewProps; @@ -61,6 +63,8 @@ public class TextLayoutManager { private static final String MAXIMUM_NUMBER_OF_LINES_KEY = "maximumNumberOfLines"; private static final LruCache sSpannableCache = new LruCache<>(spannableCacheSize); + private static final LruCache sSpannableCacheV2 = + new LruCache<>(spannableCacheSize); private static final ConcurrentHashMap sTagToSpannableCache = new ConcurrentHashMap<>(); @@ -179,20 +183,40 @@ public class TextLayoutManager { @Nullable ReactTextViewManagerCallback reactTextViewManagerCallback) { Spannable preparedSpannableText; - String attributedStringPayload = attributedString.toString(); - synchronized (sSpannableCacheLock) { - preparedSpannableText = sSpannableCache.get(attributedStringPayload); - // TODO: T31905686 implement proper equality of attributedStrings - if (preparedSpannableText != null) { - return preparedSpannableText; + String attributedStringPayload = ""; + + boolean cacheByReadableNativeMap = + ReactFeatureFlags.enableSpannableCacheByReadableNativeMapEquality; + // TODO: T74600554 Cleanup this experiment once positive impact is confirmed in production + if (cacheByReadableNativeMap) { + synchronized (sSpannableCacheLock) { + preparedSpannableText = sSpannableCacheV2.get((ReadableNativeMap) attributedString); + if (preparedSpannableText != null) { + return preparedSpannableText; + } + } + } else { + attributedStringPayload = attributedString.toString(); + synchronized (sSpannableCacheLock) { + preparedSpannableText = sSpannableCache.get(attributedStringPayload); + if (preparedSpannableText != null) { + return preparedSpannableText; + } } } preparedSpannableText = createSpannableFromAttributedString( context, attributedString, reactTextViewManagerCallback); - synchronized (sSpannableCacheLock) { - sSpannableCache.put(attributedStringPayload, preparedSpannableText); + + if (cacheByReadableNativeMap) { + synchronized (sSpannableCacheLock) { + sSpannableCacheV2.put((ReadableNativeMap) attributedString, preparedSpannableText); + } + } else { + synchronized (sSpannableCacheLock) { + sSpannableCache.put(attributedStringPayload, preparedSpannableText); + } } return preparedSpannableText; }