From 503a6f4463f5d2f7576b33158c18d8d8e99f3291 Mon Sep 17 00:00:00 2001
From: David Vacca <dvacca@fb.com>
Date: Sun, 6 Dec 2020 11:24:53 -0800
Subject: [PATCH] Optimize iteration of ReadableNativeMaps

Summary:
Props are transferred from C++ to Java using ReadableNativeMaps. The current implementation of ReadableNativeMaps creates an internal HashMap the first time one of its methods is executed.
During the update of props ReadableNativeMaps are consumed only once and they are accessed through an Iterator. That's why there is no reason to create the internal HashMap, which is inefficient from performance and memory point of view.

This diff creates an experiment that avoids the creation of the internal HashMap during prop updates, additionally it removes a lock that was being used in the creation of the internal HashMap. I expect this change to have a positive impact in TTRC and memory (in particular for ME devices)

This diff reduces the amount of ReadableNativeMaps's internal HashMaps created during initial render of MP Home by ~35%.

Changelog: [Internal]

Reviewed By: JoshuaGross

Differential Revision: D25361169

fbshipit-source-id: 7b6efda11922d56127131494ca39b5cec75f1703
---
 .../react/bridge/ReadableNativeMap.java       | 67 ++++++++++++++++++-
 .../react/config/ReactFeatureFlags.java       |  3 +
 2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReadableNativeMap.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReadableNativeMap.java
index ae471c2a8c4..d6693e1d87d 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReadableNativeMap.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReadableNativeMap.java
@@ -12,6 +12,7 @@ import androidx.annotation.Nullable;
 import com.facebook.infer.annotation.Assertions;
 import com.facebook.jni.HybridData;
 import com.facebook.proguard.annotations.DoNotStrip;
+import com.facebook.react.config.ReactFeatureFlags;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.Map;
@@ -88,6 +89,64 @@ public class ReadableNativeMap extends NativeMap implements ReadableMap {
     return mLocalTypeMap;
   }
 
+  private Iterator<Map.Entry<String, Object>> createExperimentalIterator() {
+    if (mKeys == null) {
+      mKeys = Assertions.assertNotNull(importKeys());
+    }
+    final String[] iteratorKeys = mKeys;
+    final Object[] iteratorValues = Assertions.assertNotNull(importValues());
+    return new Iterator<Map.Entry<String, Object>>() {
+      int currentIndex = 0;
+
+      @Override
+      public boolean hasNext() {
+        return currentIndex < iteratorKeys.length;
+      }
+
+      @Override
+      public Map.Entry<String, Object> next() {
+        final int index = currentIndex++;
+        return new Map.Entry<String, Object>() {
+          @Override
+          public String getKey() {
+            return iteratorKeys[index];
+          }
+
+          @Override
+          public Object getValue() {
+            return iteratorValues[index];
+          }
+
+          @Override
+          public Object setValue(Object value) {
+            throw new UnsupportedOperationException(
+                "Can't set a value while iterating over a ReadableNativeMap");
+          }
+        };
+      }
+    };
+  }
+
+  private ReadableMapKeySetIterator createExperimentalKeySetIterator() {
+    if (mKeys == null) {
+      mKeys = Assertions.assertNotNull(importKeys());
+    }
+    final String[] iteratorKeys = mKeys;
+    return new ReadableMapKeySetIterator() {
+      int currentIndex = 0;
+
+      @Override
+      public boolean hasNextKey() {
+        return currentIndex < iteratorKeys.length;
+      }
+
+      @Override
+      public String nextKey() {
+        return iteratorKeys[currentIndex++];
+      }
+    };
+  }
+
   private native Object[] importTypes();
 
   @Override
@@ -187,12 +246,16 @@ public class ReadableNativeMap extends NativeMap implements ReadableMap {
 
   @Override
   public @NonNull Iterator<Map.Entry<String, Object>> getEntryIterator() {
-    return getLocalMap().entrySet().iterator();
+    return ReactFeatureFlags.enableExperimentalReadableNativeMapIterator
+        ? createExperimentalIterator()
+        : getLocalMap().entrySet().iterator();
   }
 
   @Override
   public @NonNull ReadableMapKeySetIterator keySetIterator() {
-    return new ReadableNativeMapKeySetIterator(this);
+    return ReactFeatureFlags.enableExperimentalReadableNativeMapIterator
+        ? createExperimentalKeySetIterator()
+        : new ReadableNativeMapKeySetIterator(this);
   }
 
   @Override
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 260d366e290..c6de0646a2b 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java
@@ -97,4 +97,7 @@ public class ReactFeatureFlags {
    * we verify the fix is correct in production
    */
   public static boolean enableStartSurfaceRaceConditionFix = false;
+
+  /** Enables the usage of an experimental optimized iterator for ReadableNativeMaps. */
+  public static boolean enableExperimentalReadableNativeMapIterator = false;
 }