From 34e51191e793a88eac84ab30d65075a1e9062a59 Mon Sep 17 00:00:00 2001 From: Pieter De Baets Date: Fri, 10 Jun 2022 16:20:22 -0700 Subject: [PATCH] Optimize lookup of PlatformColor on Android Summary: We currently wrap colors in an object to make it look similar to a `PlatformColor` object, but since this is a hot codepath, let's just optimize it to a simple array of strings. The next step is to apply a layer of caching here, but this should be a simple improvement. Changelog: [internal] Reviewed By: JoshuaGross Differential Revision: D31057046 fbshipit-source-id: f68e17167ddd5bba3b545d039600c7c9b40808f5 --- .../react/bridge/ColorPropConverter.java | 49 +++++++++++-------- .../react/fabric/FabricUIManager.java | 11 +++-- .../renderer/graphics/PlatformColorParser.h | 34 +++++-------- 3 files changed, 47 insertions(+), 47 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/ColorPropConverter.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ColorPropConverter.java index 07b491dcb8..af4cfd3b18 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/ColorPropConverter.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/ColorPropConverter.java @@ -10,6 +10,7 @@ package com.facebook.react.bridge; import android.content.Context; import android.content.res.Resources; import android.util.TypedValue; +import androidx.annotation.Nullable; import androidx.core.content.res.ResourcesCompat; public class ColorPropConverter { @@ -44,27 +45,9 @@ public class ColorPropConverter { } for (int i = 0; i < resourcePaths.size(); i++) { - String resourcePath = resourcePaths.getString(i); - - if (resourcePath == null || resourcePath.isEmpty()) { - continue; - } - - boolean isResource = resourcePath.startsWith(PREFIX_RESOURCE); - boolean isThemeAttribute = resourcePath.startsWith(PREFIX_ATTR); - - resourcePath = resourcePath.substring(1); - - try { - if (isResource) { - return resolveResource(context, resourcePath); - } else if (isThemeAttribute) { - return resolveThemeAttribute(context, resourcePath); - } - } catch (Resources.NotFoundException exception) { - // The resource could not be found so do nothing to allow the for loop to continue and - // try the next fallback resource in the array. If none of the fallbacks are - // found then the exception immediately after the for loop will be thrown. + Integer result = resolveResourcePath(context, resourcePaths.getString(i)); + if (result != null) { + return result; } } @@ -78,6 +61,30 @@ public class ColorPropConverter { "ColorValue: the value must be a number or Object."); } + public static Integer resolveResourcePath(Context context, @Nullable String resourcePath) { + if (resourcePath == null || resourcePath.isEmpty()) { + return null; + } + + boolean isResource = resourcePath.startsWith(PREFIX_RESOURCE); + boolean isThemeAttribute = resourcePath.startsWith(PREFIX_ATTR); + + resourcePath = resourcePath.substring(1); + + try { + if (isResource) { + return resolveResource(context, resourcePath); + } else if (isThemeAttribute) { + return resolveThemeAttribute(context, resourcePath); + } + } catch (Resources.NotFoundException exception) { + // The resource could not be found so do nothing to allow the for loop to continue and + // try the next fallback resource in the array. If none of the fallbacks are + // found then the exception immediately after the for loop will be thrown. + } + return null; + } + private static int resolveResource(Context context, String resourcePath) { String[] pathTokens = resourcePath.split(PACKAGE_DELIMITER); diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java index d424022726..a2293980c5 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java @@ -529,11 +529,16 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { } @SuppressWarnings("unused") - public int getColor(int surfaceId, ReadableMap platformColor) { + public int getColor(int surfaceId, String[] resourcePaths) { ThemedReactContext context = mMountingManager.getSurfaceManagerEnforced(surfaceId, "getColor").getContext(); - Integer color = ColorPropConverter.getColor(platformColor, context); - return color != null ? color : 0; + for (String resourcePath : resourcePaths) { + Integer color = ColorPropConverter.resolveResourcePath(context, resourcePath); + if (color != null) { + return color; + } + } + return 0; } @SuppressWarnings("unused") diff --git a/ReactCommon/react/renderer/graphics/platform/android/react/renderer/graphics/PlatformColorParser.h b/ReactCommon/react/renderer/graphics/platform/android/react/renderer/graphics/PlatformColorParser.h index b158014071..be78b82b58 100644 --- a/ReactCommon/react/renderer/graphics/platform/android/react/renderer/graphics/PlatformColorParser.h +++ b/ReactCommon/react/renderer/graphics/platform/android/react/renderer/graphics/PlatformColorParser.h @@ -22,35 +22,23 @@ inline ColorComponents parsePlatformColor( ColorComponents colorComponents = {0, 0, 0, 0}; if (value.hasType>>()) { - auto map = (butter::map>)value; - auto resourcePaths = map["resource_paths"]; - auto dynamicResourcePaths = folly::dynamic::array(); - for (const auto &resourcePath : resourcePaths) { - dynamicResourcePaths.push_back(resourcePath); - } - folly::dynamic dynamicPlatformColor = folly::dynamic::object(); - dynamicPlatformColor["resource_paths"] = dynamicResourcePaths; - - auto fabricUIManager = + const auto &fabricUIManager = context.contextContainer.at>( "FabricUIManager"); - static auto getColorFromJava = - facebook::jni::findClassStatic( - "com/facebook/react/fabric/FabricUIManager") - ->getMethod("getColor"); + fabricUIManager->getClass() + ->getMethod)>("getColor"); - jni::local_ref dynamicPlatformColorRNM = - ReadableNativeMap::newObjectCxxArgs(dynamicPlatformColor); - jni::local_ref dynamicPlatformColorRM = - jni::make_local(reinterpret_cast( - dynamicPlatformColorRNM.get())); + auto map = (butter::map>)value; + auto &resourcePaths = map["resource_paths"]; + auto javaResourcePaths = + jni::JArrayClass::newArray(resourcePaths.size()); + for (int i = 0; i < resourcePaths.size(); i++) { + javaResourcePaths->setElement(i, *jni::make_jstring(resourcePaths[i])); + } auto color = getColorFromJava( - fabricUIManager, context.surfaceId, dynamicPlatformColorRM.get()); - - dynamicPlatformColorRM.reset(); - dynamicPlatformColorRNM.reset(); + fabricUIManager, context.surfaceId, *javaResourcePaths); auto argb = (int64_t)color; auto ratio = 255.f;