From b3790d283fbf850f9f7fef19b86085878b3d6c1d Mon Sep 17 00:00:00 2001 From: David Vacca Date: Thu, 28 Feb 2019 00:01:21 -0800 Subject: [PATCH] Back out "[Fabric][C++][Android] update props during pre allocation of views" Summary: This is a back-out of D14214844, we noticed that this regressed TTI for Marketplace You screen running in Fabric Original commit changeset: b81005f2bf49 Reviewed By: JoshuaGross Differential Revision: D14247897 fbshipit-source-id: de0cea92b437b2fbcd075f0d6a0066156800e3f0 --- React/Fabric/RCTScheduler.mm | 9 +++++++-- .../facebook/react/fabric/FabricUIManager.java | 4 ++-- .../facebook/react/fabric/jsi/jni/Binding.cpp | 18 +++++------------- .../facebook/react/fabric/jsi/jni/Binding.h | 3 ++- .../react/fabric/mounting/MountingManager.java | 15 ++------------- .../mountitems/PreAllocateViewMountItem.java | 9 ++------- ReactCommon/fabric/uimanager/Scheduler.cpp | 4 ++-- .../fabric/uimanager/SchedulerDelegate.h | 3 ++- 8 files changed, 24 insertions(+), 41 deletions(-) diff --git a/React/Fabric/RCTScheduler.mm b/React/Fabric/RCTScheduler.mm index 1cd0c9443d6..2b9961e395e 100644 --- a/React/Fabric/RCTScheduler.mm +++ b/React/Fabric/RCTScheduler.mm @@ -29,13 +29,18 @@ public: [scheduler.delegate schedulerDidFinishTransaction:mutations rootTag:rootTag]; } - void schedulerDidRequestPreliminaryViewAllocation(SurfaceId surfaceId, bool isLayoutable, const ShadowView &shadowView) override { + void schedulerDidRequestPreliminaryViewAllocation( + SurfaceId surfaceId, + ComponentName componentName, + bool isLayoutable, + ComponentHandle componentHandle) override + { if (!isLayoutable) { return; } RCTScheduler *scheduler = (__bridge RCTScheduler *)scheduler_; - [scheduler.delegate schedulerOptimisticallyCreateComponentViewWithComponentHandle:shadowView.componentHandle]; + [scheduler.delegate schedulerOptimisticallyCreateComponentViewWithComponentHandle:componentHandle]; } private: 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 58cdcd7d970..95a97a89900 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java @@ -188,7 +188,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { } @DoNotStrip - private void preallocateView(int rootTag, int reactTag, final String componentName, ReadableMap props) { + private void preallocateView(final int rootTag, final String componentName) { if (UiThreadUtil.isOnUiThread()) { // There is no reason to allocate views ahead of time on the main thread. return; @@ -198,7 +198,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { Assertions.assertNotNull(mReactContextForRootTag.get(rootTag)); String component = sComponentNames.get(componentName); Assertions.assertNotNull(component); - mPreMountItems.add(new PreAllocateViewMountItem(context, rootTag, reactTag, component, props)); + mPreMountItems.add(new PreAllocateViewMountItem(context, rootTag, component)); } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jsi/jni/Binding.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jsi/jni/Binding.cpp index 5e29c0cd4af..a28c6388ce3 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jsi/jni/Binding.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jsi/jni/Binding.cpp @@ -461,24 +461,16 @@ void Binding::setPixelDensity(float pointScaleFactor) { void Binding::schedulerDidRequestPreliminaryViewAllocation( const SurfaceId surfaceId, + const ComponentName componentName, bool isLayoutable, - const ShadowView &shadowView) { + const ComponentHandle componentHandle) { if (isLayoutable) { static auto preallocateView = jni::findClassStatic(UIManagerJavaDescriptor) - ->getMethod("preallocateView"); + ->getMethod("preallocateView"); - // TODO: T31905686 Experiment to check what is the impact on TTI of pre loading of Images during - // pre-allocation of views - if (shadowView.componentName == "Image") { - local_ref readableMap = - castReadableMap(ReadableNativeMap::newObjectCxxArgs(shadowView.props->rawProps)); - preallocateView( - javaUIManager_, surfaceId, shadowView.tag, make_jstring(shadowView.componentName).get(), readableMap.get()); - } else { - preallocateView( - javaUIManager_, surfaceId, shadowView.tag, make_jstring(shadowView.componentName).get(), nullptr); - } + preallocateView( + javaUIManager_, surfaceId, make_jstring(componentName).get()); } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jsi/jni/Binding.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jsi/jni/Binding.h index 3e3d80fb6db..ad12aa53955 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jsi/jni/Binding.h +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jsi/jni/Binding.h @@ -64,8 +64,9 @@ class Binding : public jni::HybridClass, public SchedulerDelegate { void schedulerDidRequestPreliminaryViewAllocation( const SurfaceId surfaceId, + const ComponentName componentName, bool isLayoutable, - const ShadowView &shadowView); + const ComponentHandle componentHandle); void setPixelDensity(float pointScaleFactor); diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java index 02143b21835..299b0f00ab8 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java @@ -172,10 +172,6 @@ public class MountingManager { UiThreadUtil.assertOnUiThread(); View view = null; ViewManager viewManager = null; - - // This can be possible if the view was already pre-allocated - if (mTagToViewState.get(reactTag) != null) return; - if (!isVirtual) { viewManager = mViewManagerRegistry.get(componentName); view = mViewPool.getOrCreateView(componentName, themedReactContext); @@ -275,15 +271,8 @@ public class MountingManager { } @UiThread - public void preallocateView( - ThemedReactContext reactContext, - String componentName, - int reactTag, - ReadableMap props) { - createView(reactContext, componentName, reactTag, false); - if (props != null) { - updateProps(reactTag, props); - } + public void preallocateView(ThemedReactContext reactContext, String componentName) { + mViewPool.createView(reactContext, componentName); } @UiThread diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java index 6ca1d21db15..858faf1fe6e 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java @@ -6,7 +6,6 @@ */ package com.facebook.react.fabric.mounting.mountitems; -import com.facebook.react.bridge.ReadableMap; import com.facebook.react.fabric.mounting.MountingManager; import com.facebook.react.uimanager.ThemedReactContext; @@ -17,21 +16,17 @@ public class PreAllocateViewMountItem implements MountItem { private final String mComponent; private final int mRootTag; - private final int mReactTag; - private final ReadableMap mProps; private final ThemedReactContext mContext; - public PreAllocateViewMountItem(ThemedReactContext context, int rootTag, int reactTag, String component, ReadableMap props){ + public PreAllocateViewMountItem(ThemedReactContext context, int rootTag, String component){ mContext = context; mComponent = component; mRootTag = rootTag; - mProps = props; - mReactTag = reactTag; } @Override public void execute(MountingManager mountingManager) { - mountingManager.preallocateView(mContext, mComponent, mReactTag, mProps); + mountingManager.preallocateView(mContext, mComponent); } @Override diff --git a/ReactCommon/fabric/uimanager/Scheduler.cpp b/ReactCommon/fabric/uimanager/Scheduler.cpp index 22393941de3..952428fbe4d 100644 --- a/ReactCommon/fabric/uimanager/Scheduler.cpp +++ b/ReactCommon/fabric/uimanager/Scheduler.cpp @@ -258,11 +258,11 @@ void Scheduler::uiManagerDidCreateShadowNode( dynamic_cast(shadowNode.get()); auto isLayoutable = layoutableShadowNode != nullptr; - auto shadowView = ShadowView(*shadowNode); delegate_->schedulerDidRequestPreliminaryViewAllocation( shadowNode->getRootTag(), + shadowNode->getComponentName(), isLayoutable, - shadowView); + shadowNode->getComponentHandle()); } } diff --git a/ReactCommon/fabric/uimanager/SchedulerDelegate.h b/ReactCommon/fabric/uimanager/SchedulerDelegate.h index 7976005c647..e62c31b7fa8 100644 --- a/ReactCommon/fabric/uimanager/SchedulerDelegate.h +++ b/ReactCommon/fabric/uimanager/SchedulerDelegate.h @@ -35,8 +35,9 @@ class SchedulerDelegate { */ virtual void schedulerDidRequestPreliminaryViewAllocation( SurfaceId surfaceId, + ComponentName componentName, bool isLayoutable, - const ShadowView &shadowView) = 0; + ComponentHandle componentHandle) = 0; virtual ~SchedulerDelegate() noexcept = default; };