From 68c6d71cea4e37f0360453be6e7f792ed12f6f3a Mon Sep 17 00:00:00 2001 From: Andy Street Date: Wed, 23 Nov 2016 05:12:55 -0800 Subject: [PATCH] BREAKING [react_native] Don't create CSSNodes for virtual shadow nodes Summary: Virtual shadow nodes (e.g. text) don't use CSSNodes so we don't need to create them. This shows large savings in CSSNodes allocated, depending on the app. This could be breaking if: - You have virtual nodes that still set and get CSS properties. The setters now no-op for virtual nodes (I unfortunately couldn't remove them completely -- see the comment on LayoutShadowNode), but the getters will NPE. If you see these NPE's, you should almost definitely be using your own datastructure instead of a CSSNode as virtual nodes will not participate in the layout process (and the CSSNode is then behaving just as a POJO for you). I do not anticipate this to be breaking for anyone, but am including breaking in the commit message since this is a change in API contract. Reviewed By: emilsjolander Differential Revision: D4220204 fbshipit-source-id: b8dc083fff420eb94180f669dd49389136111ecb --- .../react/uimanager/LayoutShadowNode.java | 68 +++++++++++++++++++ .../react/uimanager/ReactShadowNode.java | 43 ++++++++---- .../react/views/text/ReactRawTextManager.java | 7 +- .../react/views/text/ReactTextShadowNode.java | 19 ++---- .../views/text/ReactTextViewManager.java | 2 +- .../text/ReactVirtualTextShadowNode.java | 14 ++++ ...coBasedReactTextInlineImageShadowNode.java | 21 +++++- .../textinput/ReactTextInputShadowNode.java | 1 - 8 files changed, 139 insertions(+), 36 deletions(-) create mode 100644 ReactAndroid/src/main/java/com/facebook/react/views/text/ReactVirtualTextShadowNode.java diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/LayoutShadowNode.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/LayoutShadowNode.java index 06e039eb7..765d2cbec 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/LayoutShadowNode.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/LayoutShadowNode.java @@ -19,66 +19,104 @@ import com.facebook.react.uimanager.annotations.ReactPropGroup; /** * Supply setters for base view layout properties such as width, height, flex properties, * borders, etc. + * + * Checking for isVirtual everywhere is a hack to get around the fact that some virtual nodes still + * have layout properties set on them in JS: for example, a component that returns a may + * or may not be embedded in a parent text. There are better solutions that should probably be + * explored, namely using the VirtualText class in JS and setting the correct set of validAttributes */ public class LayoutShadowNode extends ReactShadowNode { @ReactProp(name = ViewProps.WIDTH, defaultFloat = CSSConstants.UNDEFINED) public void setWidth(float width) { + if (isVirtual()) { + return; + } setStyleWidth(CSSConstants.isUndefined(width) ? width : PixelUtil.toPixelFromDIP(width)); } @ReactProp(name = ViewProps.MIN_WIDTH, defaultFloat = CSSConstants.UNDEFINED) public void setMinWidth(float minWidth) { + if (isVirtual()) { + return; + } setStyleMinWidth( CSSConstants.isUndefined(minWidth) ? minWidth : PixelUtil.toPixelFromDIP(minWidth)); } @ReactProp(name = ViewProps.MAX_WIDTH, defaultFloat = CSSConstants.UNDEFINED) public void setMaxWidth(float maxWidth) { + if (isVirtual()) { + return; + } setStyleMaxWidth( CSSConstants.isUndefined(maxWidth) ? maxWidth : PixelUtil.toPixelFromDIP(maxWidth)); } @ReactProp(name = ViewProps.HEIGHT, defaultFloat = CSSConstants.UNDEFINED) public void setHeight(float height) { + if (isVirtual()) { + return; + } setStyleHeight( CSSConstants.isUndefined(height) ? height : PixelUtil.toPixelFromDIP(height)); } @ReactProp(name = ViewProps.MIN_HEIGHT, defaultFloat = CSSConstants.UNDEFINED) public void setMinHeight(float minHeight) { + if (isVirtual()) { + return; + } setStyleMinHeight( CSSConstants.isUndefined(minHeight) ? minHeight : PixelUtil.toPixelFromDIP(minHeight)); } @ReactProp(name = ViewProps.MAX_HEIGHT, defaultFloat = CSSConstants.UNDEFINED) public void setMaxHeight(float maxHeight) { + if (isVirtual()) { + return; + } setStyleMaxHeight( CSSConstants.isUndefined(maxHeight) ? maxHeight : PixelUtil.toPixelFromDIP(maxHeight)); } @ReactProp(name = ViewProps.FLEX, defaultFloat = 0f) public void setFlex(float flex) { + if (isVirtual()) { + return; + } super.setFlex(flex); } @ReactProp(name = ViewProps.FLEX_GROW, defaultFloat = 0f) public void setFlexGrow(float flexGrow) { + if (isVirtual()) { + return; + } super.setFlexGrow(flexGrow); } @ReactProp(name = ViewProps.FLEX_SHRINK, defaultFloat = 0f) public void setFlexShrink(float flexShrink) { + if (isVirtual()) { + return; + } super.setFlexShrink(flexShrink); } @ReactProp(name = ViewProps.FLEX_BASIS, defaultFloat = 0f) public void setFlexBasis(float flexBasis) { + if (isVirtual()) { + return; + } super.setFlexBasis(flexBasis); } @ReactProp(name = ViewProps.FLEX_DIRECTION) public void setFlexDirection(@Nullable String flexDirection) { + if (isVirtual()) { + return; + } setFlexDirection( flexDirection == null ? CSSFlexDirection.COLUMN : CSSFlexDirection.valueOf( flexDirection.toUpperCase(Locale.US).replace("-", "_"))); @@ -86,6 +124,9 @@ public class LayoutShadowNode extends ReactShadowNode { @ReactProp(name = ViewProps.FLEX_WRAP) public void setFlexWrap(@Nullable String flexWrap) { + if (isVirtual()) { + return; + } if (flexWrap == null || flexWrap.equals("nowrap")) { setFlexWrap(CSSWrap.NO_WRAP); } else if (flexWrap.equals("wrap")) { @@ -97,12 +138,18 @@ public class LayoutShadowNode extends ReactShadowNode { @ReactProp(name = ViewProps.ALIGN_SELF) public void setAlignSelf(@Nullable String alignSelf) { + if (isVirtual()) { + return; + } setAlignSelf(alignSelf == null ? CSSAlign.AUTO : CSSAlign.valueOf( alignSelf.toUpperCase(Locale.US).replace("-", "_"))); } @ReactProp(name = ViewProps.ALIGN_ITEMS) public void setAlignItems(@Nullable String alignItems) { + if (isVirtual()) { + return; + } setAlignItems( alignItems == null ? CSSAlign.STRETCH : CSSAlign.valueOf( alignItems.toUpperCase(Locale.US).replace("-", "_"))); @@ -110,12 +157,18 @@ public class LayoutShadowNode extends ReactShadowNode { @ReactProp(name = ViewProps.JUSTIFY_CONTENT) public void setJustifyContent(@Nullable String justifyContent) { + if (isVirtual()) { + return; + } setJustifyContent(justifyContent == null ? CSSJustify.FLEX_START : CSSJustify.valueOf( justifyContent.toUpperCase(Locale.US).replace("-", "_"))); } @ReactProp(name = ViewProps.OVERFLOW) public void setOverflow(@Nullable String overflow) { + if (isVirtual()) { + return; + } setOverflow(overflow == null ? CSSOverflow.VISIBLE : CSSOverflow.valueOf( overflow.toUpperCase(Locale.US).replace("-", "_"))); } @@ -130,6 +183,9 @@ public class LayoutShadowNode extends ReactShadowNode { ViewProps.MARGIN_BOTTOM, }, defaultFloat = CSSConstants.UNDEFINED) public void setMargins(int index, float margin) { + if (isVirtual()) { + return; + } setMargin(ViewProps.PADDING_MARGIN_SPACING_TYPES[index], PixelUtil.toPixelFromDIP(margin)); } @@ -143,6 +199,9 @@ public class LayoutShadowNode extends ReactShadowNode { ViewProps.PADDING_BOTTOM, }, defaultFloat = CSSConstants.UNDEFINED) public void setPaddings(int index, float padding) { + if (isVirtual()) { + return; + } setPadding( ViewProps.PADDING_MARGIN_SPACING_TYPES[index], CSSConstants.isUndefined(padding) ? padding : PixelUtil.toPixelFromDIP(padding)); @@ -156,6 +215,9 @@ public class LayoutShadowNode extends ReactShadowNode { ViewProps.BORDER_BOTTOM_WIDTH, }, defaultFloat = CSSConstants.UNDEFINED) public void setBorderWidths(int index, float borderWidth) { + if (isVirtual()) { + return; + } setBorder(ViewProps.BORDER_SPACING_TYPES[index], PixelUtil.toPixelFromDIP(borderWidth)); } @@ -166,6 +228,9 @@ public class LayoutShadowNode extends ReactShadowNode { ViewProps.BOTTOM, }, defaultFloat = CSSConstants.UNDEFINED) public void setPositionValues(int index, float position) { + if (isVirtual()) { + return; + } setPosition( ViewProps.POSITION_SPACING_TYPES[index], CSSConstants.isUndefined(position) ? position : PixelUtil.toPixelFromDIP(position)); @@ -173,6 +238,9 @@ public class LayoutShadowNode extends ReactShadowNode { @ReactProp(name = ViewProps.POSITION) public void setPosition(@Nullable String position) { + if (isVirtual()) { + return; + } CSSPositionType positionType = position == null ? CSSPositionType.RELATIVE : CSSPositionType.valueOf(position.toUpperCase(Locale.US)); setPositionType(positionType); diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactShadowNode.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactShadowNode.java index 6193b187b..8366843bb 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactShadowNode.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactShadowNode.java @@ -77,11 +77,15 @@ public class ReactShadowNode { private final CSSNode mCSSNode; public ReactShadowNode() { - CSSNode node = CSSNodePool.get().acquire(); - if (node == null) { - node = new CSSNode(); + if (!isVirtual()) { + CSSNode node = CSSNodePool.get().acquire(); + if (node == null) { + node = new CSSNode(); + } + mCSSNode = node; + } else { + mCSSNode = null; } - mCSSNode = node; } /** @@ -108,7 +112,7 @@ public class ReactShadowNode { } public final boolean hasUpdates() { - return mNodeUpdated || hasNewLayout() || mCSSNode.isDirty(); + return mNodeUpdated || hasNewLayout() || isDirty(); } public final void markUpdateSeen() { @@ -139,6 +143,10 @@ public class ReactShadowNode { } } + public final boolean isDirty() { + return mCSSNode != null && mCSSNode.isDirty(); + } + public void addChildAt(ReactShadowNode child, int i) { if (child.mParent != null) { throw new IllegalViewOperationException( @@ -152,8 +160,13 @@ public class ReactShadowNode { // If a CSS node has measure defined, the layout algorithm will not visit its children. Even // more, it asserts that you don't add children to nodes with measure functions. - if (!mCSSNode.isMeasureDefined()) { - mCSSNode.addChildAt(child.mCSSNode, i); + if (mCSSNode != null && !mCSSNode.isMeasureDefined()) { + CSSNode childCSSNode = child.mCSSNode; + if (childCSSNode == null) { + throw new RuntimeException( + "Cannot add a child that doesn't have a CSS node to a node without a measure function!"); + } + mCSSNode.addChildAt(childCSSNode, i); } markUpdated(); @@ -171,7 +184,7 @@ public class ReactShadowNode { ReactShadowNode removed = mChildren.remove(i); removed.mParent = null; - if (!mCSSNode.isMeasureDefined()) { + if (mCSSNode != null && !mCSSNode.isMeasureDefined()) { mCSSNode.removeChildAt(i); } markUpdated(); @@ -205,7 +218,7 @@ public class ReactShadowNode { int decrease = 0; for (int i = getChildCount() - 1; i >= 0; i--) { - if (!mCSSNode.isMeasureDefined()) { + if (mCSSNode != null && !mCSSNode.isMeasureDefined()) { mCSSNode.removeChildAt(i); } ReactShadowNode toRemove = getChildAt(i); @@ -344,11 +357,13 @@ public class ReactShadowNode { } public final boolean hasNewLayout() { - return mCSSNode.hasNewLayout(); + return mCSSNode == null ? false : mCSSNode.hasNewLayout(); } public final void markLayoutSeen() { - mCSSNode.markLayoutSeen(); + if (mCSSNode != null) { + mCSSNode.markLayoutSeen(); + } } /** @@ -666,7 +681,9 @@ public class ReactShadowNode { } public void dispose() { - mCSSNode.reset(); - CSSNodePool.get().release(mCSSNode); + if (mCSSNode != null) { + mCSSNode.reset(); + CSSNodePool.get().release(mCSSNode); + } } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactRawTextManager.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactRawTextManager.java index 4603183c8..339496f2a 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactRawTextManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactRawTextManager.java @@ -39,11 +39,6 @@ public class ReactRawTextManager extends ReactTextViewManager { @Override public ReactTextShadowNode createShadowNodeInstance() { - return new ReactTextShadowNode(true); - } - - @Override - public Class getShadowNodeClass() { - return ReactTextShadowNode.class; + return new ReactVirtualTextShadowNode(); } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextShadowNode.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextShadowNode.java index 95b19e815..e1e4094cd 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextShadowNode.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextShadowNode.java @@ -347,14 +347,12 @@ public class ReactTextShadowNode extends LayoutShadowNode { private @Nullable String mText = null; private @Nullable Spannable mPreparedSpannableText; - private final boolean mIsVirtual; protected boolean mContainsImages = false; private float mHeightOfTallestInlineImage = Float.NaN; - public ReactTextShadowNode(boolean isVirtual) { - mIsVirtual = isVirtual; - if (!isVirtual) { + public ReactTextShadowNode() { + if (!isVirtual()) { setMeasureFunction(mTextMeasureFunction); } } @@ -383,7 +381,7 @@ public class ReactTextShadowNode extends LayoutShadowNode { @Override public void onBeforeLayout() { - if (mIsVirtual) { + if (isVirtual()) { return; } mPreparedSpannableText = fromTextCSSNode(this); @@ -394,7 +392,7 @@ public class ReactTextShadowNode extends LayoutShadowNode { public void markUpdated() { super.markUpdated(); // We mark virtual anchor node as dirty as updated text needs to be re-measured - if (!mIsVirtual) { + if (!isVirtual()) { super.dirty(); } } @@ -566,17 +564,12 @@ public class ReactTextShadowNode extends LayoutShadowNode { @Override public boolean isVirtualAnchor() { - return !mIsVirtual; - } - - @Override - public boolean isVirtual() { - return mIsVirtual; + return !isVirtual(); } @Override public void onCollectExtraUpdates(UIViewOperationQueue uiViewOperationQueue) { - if (mIsVirtual) { + if (isVirtual()) { return; } super.onCollectExtraUpdates(uiViewOperationQueue); diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextViewManager.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextViewManager.java index 82deb5bc4..af017fcbf 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextViewManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextViewManager.java @@ -155,7 +155,7 @@ public class ReactTextViewManager extends BaseViewManager