From a636ddd9f07d8d3e6febb2118a7c7a4d225ad129 Mon Sep 17 00:00:00 2001 From: Andy Street Date: Thu, 26 Nov 2015 08:36:02 -0800 Subject: [PATCH] Backout D2677289 [react_native] View recycling in JS Summary: public We're seeing related crashes. The diff has no tests, the perf tests weren't conclusive, and the person who'd be supporting it no longer is available to work on it. We can try this again later in a less rushed manner with proper perf testing. Reviewed By: davidaurelio Differential Revision: D2696615 fb-gh-sync-id: 3b6814ac12af19516146d5c42d2add8321b10db5 --- .../ReactNative/ReactNativeBaseComponent.js | 21 +- Libraries/ReactNative/ReactNativeMount.js | 2 - .../ReactNativeReconcileTransaction.js | 9 +- Libraries/ReactNative/ReactNativeViewPool.js | 264 ------------------ .../uimanager/NativeViewHierarchyManager.java | 15 +- .../NativeViewHierarchyOptimizer.java | 4 - .../react/uimanager/UIImplementation.java | 17 +- .../react/uimanager/UIManagerModule.java | 5 - .../uimanager/UIManagerModuleConstants.java | 8 - .../UIManagerModuleConstantsHelper.java | 2 - .../react/uimanager/UIViewOperationQueue.java | 23 -- .../facebook/react/uimanager/ViewProps.java | 2 +- 12 files changed, 27 insertions(+), 345 deletions(-) delete mode 100644 Libraries/ReactNative/ReactNativeViewPool.js diff --git a/Libraries/ReactNative/ReactNativeBaseComponent.js b/Libraries/ReactNative/ReactNativeBaseComponent.js index b187a39f0..cfd3df3e8 100644 --- a/Libraries/ReactNative/ReactNativeBaseComponent.js +++ b/Libraries/ReactNative/ReactNativeBaseComponent.js @@ -16,7 +16,6 @@ var ReactNativeAttributePayload = require('ReactNativeAttributePayload'); var ReactNativeEventEmitter = require('ReactNativeEventEmitter'); var ReactNativeStyleAttributes = require('ReactNativeStyleAttributes'); var ReactNativeTagHandles = require('ReactNativeTagHandles'); -var ReactNativeViewPool = require('ReactNativeViewPool'); var ReactMultiChild = require('ReactMultiChild'); var RCTUIManager = require('NativeModules').UIManager; @@ -89,7 +88,6 @@ ReactNativeBaseComponent.Mixin = { unmountComponent: function() { deleteAllListeners(this._rootNodeID); this.unmountChildren(); - ReactNativeViewPool.release(this); this._rootNodeID = null; }, @@ -206,7 +204,24 @@ ReactNativeBaseComponent.Mixin = { mountComponent: function(rootID, transaction, context) { this._rootNodeID = rootID; - var tag = ReactNativeViewPool.acquire(this); + var tag = ReactNativeTagHandles.allocateTag(); + + if (__DEV__) { + deepFreezeAndThrowOnMutationInDev(this._currentElement.props); + } + + var updatePayload = ReactNativeAttributePayload.create( + this._currentElement.props, + this.viewConfig.validAttributes + ); + + var nativeTopRootID = ReactNativeTagHandles.getNativeTopRootIDFromNodeID(rootID); + RCTUIManager.createView( + tag, + this.viewConfig.uiViewClassName, + nativeTopRootID ? ReactNativeTagHandles.rootNodeIDToTag[nativeTopRootID] : null, + updatePayload + ); this._registerListenersUponCreation(this._currentElement.props); this.initializeChildren( diff --git a/Libraries/ReactNative/ReactNativeMount.js b/Libraries/ReactNative/ReactNativeMount.js index aeef4cba7..02dbcb40f 100644 --- a/Libraries/ReactNative/ReactNativeMount.js +++ b/Libraries/ReactNative/ReactNativeMount.js @@ -15,7 +15,6 @@ var RCTUIManager = require('NativeModules').UIManager; var ReactElement = require('ReactElement'); var ReactNativeTagHandles = require('ReactNativeTagHandles'); -var ReactNativeViewPool = require('ReactNativeViewPool'); var ReactPerf = require('ReactPerf'); var ReactReconciler = require('ReactReconciler'); var ReactUpdateQueue = require('ReactUpdateQueue'); @@ -217,7 +216,6 @@ var ReactNativeMount = { ReactNativeMount.unmountComponentAtNode(containerTag); // call back into native to remove all of the subviews from this container RCTUIManager.removeRootView(containerTag); - ReactNativeViewPool.clearPoolForRootView(containerTag); }, /** diff --git a/Libraries/ReactNative/ReactNativeReconcileTransaction.js b/Libraries/ReactNative/ReactNativeReconcileTransaction.js index aff41081d..309630e3c 100644 --- a/Libraries/ReactNative/ReactNativeReconcileTransaction.js +++ b/Libraries/ReactNative/ReactNativeReconcileTransaction.js @@ -14,7 +14,6 @@ var CallbackQueue = require('CallbackQueue'); var PooledClass = require('PooledClass'); var Transaction = require('Transaction'); -var ReactNativeViewPool = require('ReactNativeViewPool'); /** * Provides a `CallbackQueue` queue for collecting `onDOMReady` callbacks during @@ -36,18 +35,12 @@ var ON_DOM_READY_QUEUEING = { } }; -var RN_VIEW_POOL_WRAPPER = { - close: function() { - ReactNativeViewPool.onReconcileTransactionClose(); - }, -}; - /** * Executed within the scope of the `Transaction` instance. Consider these as * being member methods, but with an implied ordering while being isolated from * each other. */ -var TRANSACTION_WRAPPERS = [ON_DOM_READY_QUEUEING, RN_VIEW_POOL_WRAPPER]; +var TRANSACTION_WRAPPERS = [ON_DOM_READY_QUEUEING]; /** * Currently: diff --git a/Libraries/ReactNative/ReactNativeViewPool.js b/Libraries/ReactNative/ReactNativeViewPool.js deleted file mode 100644 index dd0a59616..000000000 --- a/Libraries/ReactNative/ReactNativeViewPool.js +++ /dev/null @@ -1,264 +0,0 @@ -/** - * Copyright (c) 2015-present, Facebook, Inc. - * All rights reserved. - * - * This source code is licensed under the BSD-style license found in the - * LICENSE file in the root directory of this source tree. An additional grant - * of patent rights can be found in the PATENTS file in the same directory. - * - * @providesModule ReactNativeViewPool - */ -'use strict'; - -var ReactNativeTagHandles = require('ReactNativeTagHandles'); -var ReactNativeAttributePayload = require('ReactNativeAttributePayload'); -var RCTUIManager = require('NativeModules').UIManager; -var Platform = require('Platform'); - -var deepFreezeAndThrowOnMutationInDev = require('deepFreezeAndThrowOnMutationInDev'); -var emptyFunction = require('emptyFunction'); -var flattenStyle = require('flattenStyle'); - -var EMPTY_POOL = [[]]; - -var ENABLED = !!RCTUIManager.dropViews; - -/* indicies used for _addToPool arrays */ -var TAGS_IDX = 0; -var KEYS_IDX = 1; -var PROPS_IDX = 2; - -var _pools = {}; -var _poolSize = {}; - -var layoutOnlyProps = RCTUIManager.layoutOnlyProps; - -function isCollapsableForStyle(style) { - var flatStyle = flattenStyle(style); - for (var styleKey in flatStyle) { - if (layoutOnlyProps[styleKey] !== true) { - return false; - } - } - return true; -} - -function isCollapsable(viewRef) { - var props = viewRef._currentElement.props; - if (props.collapsable !== undefined && !props.collapsable) { - return false; - } - var validAttributes = viewRef.viewConfig.validAttributes; - for (var propKey in props) { - if (!!validAttributes[propKey] && propKey !== 'style' && propKey !== 'collapsable') { - return false; - } - } - return !props.style || isCollapsableForStyle(viewRef._currentElement.props.style); -} - -function enqueueCreate(viewRef, rootTag) { - var tag = ReactNativeTagHandles.allocateTag(); - - if (__DEV__) { - deepFreezeAndThrowOnMutationInDev(viewRef._currentElement.props); - } - - var updatePayload = ReactNativeAttributePayload.create( - viewRef._currentElement.props, - viewRef.viewConfig.validAttributes - ); - - RCTUIManager.createView( - tag, - viewRef.viewConfig.uiViewClassName, - rootTag, - updatePayload - ); - - return tag; -} - -function getViewTag(viewRef) { - return ReactNativeTagHandles.mostRecentMountedNodeHandleForRootNodeID(viewRef._rootNodeID); -} - -function getViewProps(viewRef) { - return viewRef._currentElement.props; -} - -function getViewValidAttributes(viewRef) { - return viewRef.viewConfig.validAttributes; -} - -function getRootViewTag(viewRef) { - var nativeTopRootID = ReactNativeTagHandles.getNativeTopRootIDFromNodeID(viewRef._rootNodeID); - return ReactNativeTagHandles.rootNodeIDToTag[nativeTopRootID]; -} - -function poolKey(viewRef) { - var viewClass = viewRef.viewConfig.uiViewClassName; - if (Platform.OS === 'android' && viewClass === 'RCTView') { - return isCollapsable(viewRef) ? 'CollapsedRCTView' : 'RCTView'; - } - return viewClass; -} - -class ReactNativeViewPool { - constructor() { - this._pool = {}; - this._poolQueue = {}; - this._addToPool = [[],[],[]]; - this._viewsToDelete = []; - if (__DEV__) { - this._recycleStats = {}; - this._deleteStats = {}; - } - } - - onReconcileTransactionClose() { - // flush all deletes, move object from pool_queue to the actual pool - if (this._viewsToDelete.length > 0) { - RCTUIManager.dropViews(this._viewsToDelete); - } - var addToPoolTags = this._addToPool[TAGS_IDX]; - var addToPoolKeys = this._addToPool[KEYS_IDX]; - var addToPoolProps = this._addToPool[PROPS_IDX]; - for (var i = addToPoolTags.length - 1; i >= 0; i--) { - var nativeTag = addToPoolTags[i]; - var key = addToPoolKeys[i]; - var props = addToPoolProps[i]; - var views = this._pool[key] || [[],[]]; - views[0].push(nativeTag); - views[1].push(props); - this._pool[key] = views; - } - this._viewsToDelete = []; - this._addToPool = [[],[],[]]; - this._poolQueue = {}; - } - - acquire(viewRef, rootTag) { - var key = poolKey(viewRef); - if ((this._pool[key] || EMPTY_POOL)[0].length) { - var views = this._pool[key]; - var nativeTag = views[0].pop(); - var oldProps = views[1].pop(); - var updatePayload = ReactNativeAttributePayload.diff( - oldProps, - getViewProps(viewRef), - getViewValidAttributes(viewRef) - ); - if (__DEV__) { - this._recycleStats[key] = (this._recycleStats[key] || 0) + 1; - } - - if (updatePayload) { - RCTUIManager.updateView( - nativeTag, - viewRef.viewConfig.uiViewClassName, - updatePayload - ); - } - return nativeTag; - } else { - // If there is no view available for the given pool key, we just enqueue call to create one - return enqueueCreate(viewRef, rootTag); - } - } - - release(viewRef) { - var key = poolKey(viewRef); - var nativeTag = getViewTag(viewRef); - var pooledCount = (this._pool[key] || EMPTY_POOL)[0].length + (this._poolQueue[key] || 0); - if (pooledCount < (_poolSize[key] || 0)) { - // we have room in the pool for this view - // we can add it to the queue so that it will be added to the actual pull in - // onReconcileTransactionClose - this._addToPool[TAGS_IDX].push(nativeTag); - this._addToPool[KEYS_IDX].push(key); - this._addToPool[PROPS_IDX].push(getViewProps(viewRef)); - this._poolQueue[key] = (this._poolQueue[key] || 0) + 1; - } else { - if (__DEV__) { - if (_poolSize[key]) { - this._deleteStats[key] = (this._deleteStats[key] || 0) + 1; - } - } - this._viewsToDelete.push(nativeTag); - } - } - - clear() { - for (var key in this._pool) { - var poolTags = this._pool[key][0]; - for (var i = poolTags.length - 1; i >= 0; i--) { - this._viewsToDelete.push(poolTags[i]); - } - } - var addToPoolTags = this._addToPool[0]; - for (var i = addToPoolTags.length - 1; i >= 0; i--) { - this._viewsToDelete.push(addToPoolTags[i]); - } - this._addToPool = [[],[],[]]; - this.onReconcileTransactionClose(); - } - - printStats() { - if (__DEV__) { - console.log('Stats', this._recycleStats, this._deleteStats); - } - } -} - -module.exports = { - - onReconcileTransactionClose: function() { - if (ENABLED) { - for (var pool in _pools) { - _pools[pool].onReconcileTransactionClose(); - } - } - }, - - acquire: function(viewRef) { - var rootTag = getRootViewTag(viewRef); - if (ENABLED) { - var pool = _pools[rootTag]; - if (!pool) { - pool = _pools[rootTag] = new ReactNativeViewPool(); - } - return pool.acquire(viewRef, rootTag); - } else { - return enqueueCreate(viewRef, rootTag); - } - }, - - release: ENABLED ? function(viewRef) { - var pool = _pools[getRootViewTag(viewRef)]; - if (pool) { - pool.release(viewRef); - } - } : emptyFunction, - - clearPoolForRootView: ENABLED ? function(rootID) { - var pool = _pools[rootID]; - if (pool) { - pool.clear(); - delete _pools[rootID]; - } - } : emptyFunction, - - configure: function(pool_size) { - _poolSize = pool_size; - }, - - printStats: function() { - if (__DEV__) { - console.log('Pool size', _poolSize); - for (var pool in _pools) { - _pools[pool].onReconcileTransactionClose(); - } - } - }, -}; diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java index cf3c56455..fddd70478 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java @@ -351,7 +351,7 @@ import com.facebook.react.touch.JSResponderHandler; viewsToAdd, tagsToDelete)); } - detachView(viewToDestroy); + dropView(viewToDestroy); } } } @@ -380,15 +380,10 @@ import com.facebook.react.touch.JSResponderHandler; view.setId(tag); } - public void dropView(int tag) { - mTagsToViews.remove(tag); - mTagsToViewManagers.remove(tag); - } - /** * Releases all references to given native View. */ - private void detachView(View view) { + private void dropView(View view) { UiThreadUtil.assertOnUiThread(); if (!mRootTags.get(view.getId())) { // For non-root views we notify viewmanager with {@link ViewManager#onDropInstance} @@ -403,11 +398,13 @@ import com.facebook.react.touch.JSResponderHandler; for (int i = viewGroupManager.getChildCount(viewGroup) - 1; i >= 0; i--) { View child = viewGroupManager.getChildAt(viewGroup, i); if (mTagsToViews.get(child.getId()) != null) { - detachView(child); + dropView(child); } } viewGroupManager.removeAllViews(viewGroup); } + mTagsToViews.remove(view.getId()); + mTagsToViewManagers.remove(view.getId()); } public void removeRootView(int rootViewTag) { @@ -417,7 +414,7 @@ import com.facebook.react.touch.JSResponderHandler; "View with tag " + rootViewTag + " is not registered as a root view"); } View rootView = mTagsToViews.get(rootViewTag); - detachView(rootView); + dropView(rootView); mRootTags.delete(rootViewTag); mRootViewsContext.remove(rootViewTag); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyOptimizer.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyOptimizer.java index e01fab28d..fd3946eeb 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyOptimizer.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyOptimizer.java @@ -90,10 +90,6 @@ public class NativeViewHierarchyOptimizer { } } - public void handleDropViews(int[] viewTagsToDrop, int length) { - mUIViewOperationQueue.enqueueDropViews(viewTagsToDrop, length); - } - /** * Handles native children cleanup when css node is removed from hierarchy */ diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java index b9eb9a64f..1718fe2bb 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java @@ -124,22 +124,6 @@ public class UIImplementation { } } - public void dropViews(ReadableArray viewTags) { - int size = viewTags.size(), realViewsCount = 0; - int realViewTags[] = new int[size]; - for (int i = 0; i < size; i++) { - int tag = viewTags.getInt(i); - ReactShadowNode cssNode = mShadowNodeRegistry.getNode(tag); - if (!cssNode.isVirtual()) { - realViewTags[realViewsCount++] = tag; - } - mShadowNodeRegistry.removeNode(tag); - } - if (realViewsCount > 0) { - mNativeViewHierarchyOptimizer.handleDropViews(realViewTags, realViewsCount); - } - } - /** * Invoked by React to create a new node with a given tag has its properties changed. */ @@ -509,6 +493,7 @@ public class UIImplementation { private void removeShadowNode(ReactShadowNode nodeToRemove) { mNativeViewHierarchyOptimizer.handleRemoveNode(nodeToRemove); + mShadowNodeRegistry.removeNode(nodeToRemove.getReactTag()); for (int i = nodeToRemove.getChildCount() - 1; i >= 0; i--) { removeShadowNode(nodeToRemove.getChildAt(i)); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java index ff473aaf6..04724229b 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java @@ -202,11 +202,6 @@ public class UIManagerModule extends ReactContextBaseJavaModule implements mUIImplementation.createView(tag, className, rootViewTag, props); } - @ReactMethod - public void dropViews(ReadableArray viewTags) { - mUIImplementation.dropViews(viewTags); - } - @ReactMethod public void updateView(int tag, String className, ReadableMap props) { mUIImplementation.updateView(tag, className, props); diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModuleConstants.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModuleConstants.java index eab1c3489..3287ebf78 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModuleConstants.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModuleConstants.java @@ -154,12 +154,4 @@ import com.facebook.react.uimanager.events.TouchEventType; return constants; } - - public static Map getLayoutOnlyPropsConstants() { - HashMap constants = new HashMap<>(); - for (String propName : ViewProps.LAYOUT_ONLY_PROPS) { - constants.put(propName, Boolean.TRUE); - } - return constants; - } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModuleConstantsHelper.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModuleConstantsHelper.java index d5b566529..bce7c3736 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModuleConstantsHelper.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModuleConstantsHelper.java @@ -25,7 +25,6 @@ import com.facebook.react.common.MapBuilder; private static final String CUSTOM_BUBBLING_EVENT_TYPES_KEY = "customBubblingEventTypes"; private static final String CUSTOM_DIRECT_EVENT_TYPES_KEY = "customDirectEventTypes"; - private static final String LAYOUT_ONLY_PROPS = "layoutOnlyProps"; /** * Generates map of constants that is then exposed by {@link UIManagerModule}. The constants map @@ -76,7 +75,6 @@ import com.facebook.react.common.MapBuilder; constants.put(CUSTOM_BUBBLING_EVENT_TYPES_KEY, bubblingEventTypesConstants); constants.put(CUSTOM_DIRECT_EVENT_TYPES_KEY, directEventTypesConstants); - constants.put(LAYOUT_ONLY_PROPS, UIManagerModuleConstants.getLayoutOnlyPropsConstants()); return constants; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIViewOperationQueue.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIViewOperationQueue.java index 21f1128d8..ed507fd3e 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIViewOperationQueue.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIViewOperationQueue.java @@ -147,25 +147,6 @@ public class UIViewOperationQueue { } } - private final class DropViewsOperation extends ViewOperation { - - private final int[] mViewTagsToDrop; - private final int mArrayLength; - - public DropViewsOperation(int[] viewTagsToDrop, int length) { - super(-1); - mViewTagsToDrop = viewTagsToDrop; - mArrayLength = length; - } - - @Override - public void execute() { - for (int i = 0; i < mArrayLength; i++) { - mNativeViewHierarchyManager.dropView(mViewTagsToDrop[i]); - } - } - } - private final class ManageChildrenOperation extends ViewOperation { private final @Nullable int[] mIndicesToRemove; @@ -556,10 +537,6 @@ public class UIViewOperationQueue { initialProps)); } - public void enqueueDropViews(int[] viewTagsToDrop, int length) { - mOperations.add(new DropViewsOperation(viewTagsToDrop, length)); - } - public void enqueueUpdateProperties(int reactTag, String className, CatalystStylesDiffMap props) { mOperations.add(new UpdatePropertiesOperation(reactTag, props)); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewProps.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewProps.java index d19ea165e..7e87a8419 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewProps.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewProps.java @@ -84,7 +84,7 @@ public class ViewProps { Spacing.BOTTOM }; - /*package*/ static final HashSet LAYOUT_ONLY_PROPS = new HashSet<>( + private static final HashSet LAYOUT_ONLY_PROPS = new HashSet<>( Arrays.asList( ALIGN_SELF, ALIGN_ITEMS,