From 1baa239389ca3943ea952d493163b13e39a74d8c Mon Sep 17 00:00:00 2001 From: Emil Sjolander Date: Tue, 8 Nov 2016 09:07:48 -0800 Subject: [PATCH] Remove isTextNode optimization Summary: Scrolling through feed and logging when this optimization is hit leads to... 0 logs. If anything this just adds to confusion. It was initially added due to instinct and not data, which was a mistake. I am happy to add some similar optimization in the future if we have data that it is useful in real world situations, currently it just leads to bugs and confusion though. Reviewed By: astreet Differential Revision: D4146785 fbshipit-source-id: e20d780fbd5759b8f38b809e8cadf29cedee82a8 --- CSSLayout/CSSLayout.c | 13 ++----- CSSLayout/CSSLayout.h | 4 +- csharp/Facebook.CSSLayout/CSSNode.cs | 13 ------- csharp/Facebook.CSSLayout/Native.cs | 7 ---- java/com/facebook/csslayout/CSSNode.java | 12 ------ java/com/facebook/csslayout/CSSNodeAPI.java | 2 - .../facebook/csslayout/CSSNodeDEPRECATED.java | 11 ------ java/com/facebook/csslayout/LayoutEngine.java | 37 +------------------ java/jni/CSSJNI.cpp | 10 ----- tests/CSSLayoutMeasureCacheTest.cpp | 20 ---------- 10 files changed, 7 insertions(+), 122 deletions(-) diff --git a/CSSLayout/CSSLayout.c b/CSSLayout/CSSLayout.c index ebde96c0..2e2b9960 100644 --- a/CSSLayout/CSSLayout.c +++ b/CSSLayout/CSSLayout.c @@ -89,7 +89,6 @@ typedef struct CSSNode { CSSLayout layout; uint32_t lineIndex; bool hasNewLayout; - bool isTextNode; CSSNodeRef parent; CSSNodeListRef children; bool isDirty; @@ -382,7 +381,6 @@ void CSSNodeStyleSetFlex(const CSSNodeRef node, const float flex) { CSS_NODE_PROPERTY_IMPL(void *, Context, context, context); CSS_NODE_PROPERTY_IMPL(CSSPrintFunc, PrintFunc, printFunc, print); -CSS_NODE_PROPERTY_IMPL(bool, IsTextnode, isTextNode, isTextNode); CSS_NODE_PROPERTY_IMPL(bool, HasNewLayout, hasNewLayout, hasNewLayout); CSS_NODE_STYLE_PROPERTY_IMPL(CSSDirection, Direction, direction, direction); @@ -2104,8 +2102,7 @@ static inline bool newMeasureSizeIsStricterAndStillValid(CSSMeasureMode sizeMode lastSize > size && lastComputedSize <= size; } -bool CSSNodeCanUseCachedMeasurement(const bool isTextNode, - const CSSMeasureMode widthMode, +bool CSSNodeCanUseCachedMeasurement(const CSSMeasureMode widthMode, const float width, const CSSMeasureMode heightMode, const float height, @@ -2134,7 +2131,7 @@ bool CSSNodeCanUseCachedMeasurement(const bool isTextNode, newMeasureSizeIsStricterAndStillValid( widthMode, width - marginRow, lastWidthMode, lastWidth, lastComputedWidth); - const bool heightIsCompatible = isTextNode || hasSameHeightSpec || + const bool heightIsCompatible = hasSameHeightSpec || newSizeIsExactAndMatchesOldMeasuredSize(heightMode, height - marginColumn, lastComputedHeight) || @@ -2203,8 +2200,7 @@ bool layoutNodeInternal(const CSSNodeRef node, const float marginAxisColumn = getMarginAxis(node, CSSFlexDirectionColumn); // First, try to use the layout cache. - if (CSSNodeCanUseCachedMeasurement(node->isTextNode, - widthMeasureMode, + if (CSSNodeCanUseCachedMeasurement(widthMeasureMode, availableWidth, heightMeasureMode, availableHeight, @@ -2220,8 +2216,7 @@ bool layoutNodeInternal(const CSSNodeRef node, } else { // Try to use the measurement cache. for (uint32_t i = 0; i < layout->nextCachedMeasurementsIndex; i++) { - if (CSSNodeCanUseCachedMeasurement(node->isTextNode, - widthMeasureMode, + if (CSSNodeCanUseCachedMeasurement(widthMeasureMode, availableWidth, heightMeasureMode, availableHeight, diff --git a/CSSLayout/CSSLayout.h b/CSSLayout/CSSLayout.h index 881d8f3b..88272ad7 100644 --- a/CSSLayout/CSSLayout.h +++ b/CSSLayout/CSSLayout.h @@ -161,8 +161,7 @@ WIN_EXPORT void CSSNodePrint(const CSSNodeRef node, const CSSPrintOptions option WIN_EXPORT bool CSSValueIsUndefined(const float value); -WIN_EXPORT bool CSSNodeCanUseCachedMeasurement(const bool isTextNode, - const CSSMeasureMode widthMode, +WIN_EXPORT bool CSSNodeCanUseCachedMeasurement(const CSSMeasureMode widthMode, const float width, const CSSMeasureMode heightMode, const float height, @@ -195,7 +194,6 @@ WIN_EXPORT bool CSSNodeCanUseCachedMeasurement(const bool isTextNode, CSS_NODE_PROPERTY(void *, Context, context); CSS_NODE_PROPERTY(CSSMeasureFunc, MeasureFunc, measureFunc); CSS_NODE_PROPERTY(CSSPrintFunc, PrintFunc, printFunc); -CSS_NODE_PROPERTY(bool, IsTextnode, isTextNode); CSS_NODE_PROPERTY(bool, HasNewLayout, hasNewLayout); CSS_NODE_STYLE_PROPERTY(CSSDirection, Direction, direction); diff --git a/csharp/Facebook.CSSLayout/CSSNode.cs b/csharp/Facebook.CSSLayout/CSSNode.cs index 1648617f..b28114d5 100644 --- a/csharp/Facebook.CSSLayout/CSSNode.cs +++ b/csharp/Facebook.CSSLayout/CSSNode.cs @@ -62,19 +62,6 @@ namespace Facebook.CSSLayout Native.CSSNodeMarkDirty(_cssNode); } - public bool IsTextNode - { - get - { - return Native.CSSNodeGetIsTextnode(_cssNode); - } - - set - { - Native.CSSNodeSetIsTextnode(_cssNode, value); - } - } - public bool HasNewLayout { get diff --git a/csharp/Facebook.CSSLayout/Native.cs b/csharp/Facebook.CSSLayout/Native.cs index 163f0049..0d736f36 100644 --- a/csharp/Facebook.CSSLayout/Native.cs +++ b/csharp/Facebook.CSSLayout/Native.cs @@ -92,13 +92,6 @@ namespace Facebook.CSSLayout [return: MarshalAs(UnmanagedType.FunctionPtr)] public static extern CSSMeasureFunc CSSNodeGetMeasureFunc(IntPtr node); - [DllImport(DllName)] - public static extern void CSSNodeSetIsTextnode(IntPtr node, [MarshalAs(UnmanagedType.I1)] bool isTextNode); - - [DllImport(DllName)] - [return: MarshalAs(UnmanagedType.I1)] - public static extern bool CSSNodeGetIsTextnode(IntPtr node); - [DllImport(DllName)] public static extern void CSSNodeSetHasNewLayout(IntPtr node, [MarshalAs(UnmanagedType.I1)] bool hasNewLayout); diff --git a/java/com/facebook/csslayout/CSSNode.java b/java/com/facebook/csslayout/CSSNode.java index 6d5d61b3..612be246 100644 --- a/java/com/facebook/csslayout/CSSNode.java +++ b/java/com/facebook/csslayout/CSSNode.java @@ -140,18 +140,6 @@ public class CSSNode implements CSSNodeAPI { return mChildren == null ? -1 : mChildren.indexOf(child); } - private native void jni_CSSNodeSetIsTextNode(long nativePointer, boolean isTextNode); - @Override - public void setIsTextNode(boolean isTextNode) { - jni_CSSNodeSetIsTextNode(mNativePointer, isTextNode); - } - - private native boolean jni_CSSNodeGetIsTextNode(long nativePointer); - @Override - public boolean isTextNode() { - return jni_CSSNodeGetIsTextNode(mNativePointer); - } - private native void jni_CSSNodeCalculateLayout(long nativePointer); @Override public void calculateLayout(CSSLayoutContext layoutContext) { diff --git a/java/com/facebook/csslayout/CSSNodeAPI.java b/java/com/facebook/csslayout/CSSNodeAPI.java index 5a5de382..dd18a54b 100644 --- a/java/com/facebook/csslayout/CSSNodeAPI.java +++ b/java/com/facebook/csslayout/CSSNodeAPI.java @@ -31,8 +31,6 @@ public interface CSSNodeAPI { int indexOf(CSSNodeType child); void setMeasureFunction(MeasureFunction measureFunction); boolean isMeasureDefined(); - void setIsTextNode(boolean isTextNode); - boolean isTextNode(); void calculateLayout(CSSLayoutContext layoutContext); boolean isDirty(); boolean hasNewLayout(); diff --git a/java/com/facebook/csslayout/CSSNodeDEPRECATED.java b/java/com/facebook/csslayout/CSSNodeDEPRECATED.java index 05632cf8..dc31cc04 100644 --- a/java/com/facebook/csslayout/CSSNodeDEPRECATED.java +++ b/java/com/facebook/csslayout/CSSNodeDEPRECATED.java @@ -58,7 +58,6 @@ public class CSSNodeDEPRECATED implements CSSNodeAPI { private @Nullable CSSNodeDEPRECATED mParent; private @Nullable MeasureFunction mMeasureFunction = null; private LayoutState mLayoutState = LayoutState.DIRTY; - private boolean mIsTextNode = false; private Object mData; @Override @@ -124,16 +123,6 @@ public class CSSNodeDEPRECATED implements CSSNodeAPI { return mMeasureFunction != null; } - @Override - public void setIsTextNode(boolean isTextNode) { - mIsTextNode = isTextNode; - } - - @Override - public boolean isTextNode() { - return mIsTextNode; - } - long measure(float width, CSSMeasureMode widthMode, float height, CSSMeasureMode heightMode) { if (!isMeasureDefined()) { throw new RuntimeException("Measure function isn't defined!"); diff --git a/java/com/facebook/csslayout/LayoutEngine.java b/java/com/facebook/csslayout/LayoutEngine.java index 79d33bce..c6fa42c2 100644 --- a/java/com/facebook/csslayout/LayoutEngine.java +++ b/java/com/facebook/csslayout/LayoutEngine.java @@ -240,7 +240,6 @@ public class LayoutEngine { } /*package*/ static boolean canUseCachedMeasurement( - boolean isTextNode, float availableWidth, float availableHeight, float marginRow, @@ -281,38 +280,6 @@ public class LayoutEngine { return true; } - // We know this to be text so we can apply some more specialized heuristics. - if (isTextNode) { - if (isWidthSame) { - if (heightMeasureMode == CSSMeasureMode.UNDEFINED) { - // Width is the same and height is not restricted. Re-use cahced value. - return true; - } - - if (heightMeasureMode == CSSMeasureMode.AT_MOST && - cachedLayout.computedHeight < (availableHeight - marginColumn)) { - // Width is the same and height restriction is greater than the cached height. Re-use cached value. - return true; - } - - // Width is the same but height restriction imposes smaller height than previously measured. - // Update the cached value to respect the new height restriction. - cachedLayout.computedHeight = availableHeight - marginColumn; - return true; - } - - if (cachedLayout.widthMeasureMode == CSSMeasureMode.UNDEFINED) { - if (widthMeasureMode == CSSMeasureMode.UNDEFINED || - (widthMeasureMode == CSSMeasureMode.AT_MOST && - cachedLayout.computedWidth <= (availableWidth - marginRow))) { - // Previsouly this text was measured with no width restriction, if width is now restricted - // but to a larger value than the previsouly measured width we can re-use the measurement - // as we know it will fit. - return true; - } - } - } - return false; } @@ -364,13 +331,13 @@ public class LayoutEngine { node.style.margin.getWithFallback(trailingSpacing[CSS_FLEX_DIRECTION_COLUMN], trailing[CSS_FLEX_DIRECTION_COLUMN]); // First, try to use the layout cache. - if (canUseCachedMeasurement(node.isTextNode(), availableWidth, availableHeight, marginAxisRow, marginAxisColumn, + if (canUseCachedMeasurement(availableWidth, availableHeight, marginAxisRow, marginAxisColumn, widthMeasureMode, heightMeasureMode, layout.cachedLayout)) { cachedResults = layout.cachedLayout; } else { // Try to use the measurement cache. for (int i = 0; i < layout.nextCachedMeasurementsIndex; i++) { - if (canUseCachedMeasurement(node.isTextNode(), availableWidth, availableHeight, marginAxisRow, marginAxisColumn, + if (canUseCachedMeasurement(availableWidth, availableHeight, marginAxisRow, marginAxisColumn, widthMeasureMode, heightMeasureMode, layout.cachedMeasurements[i])) { cachedResults = layout.cachedMeasurements[i]; break; diff --git a/java/jni/CSSJNI.cpp b/java/jni/CSSJNI.cpp index 87c67cc8..e1c86237 100644 --- a/java/jni/CSSJNI.cpp +++ b/java/jni/CSSJNI.cpp @@ -129,14 +129,6 @@ void jni_CSSNodeSetHasMeasureFunc(alias_ref, CSSNodeSetMeasureFunc(_jlong2CSSNodeRef(nativePointer), hasMeasureFunc ? _jniMeasureFunc : NULL); } -void jni_CSSNodeSetIsTextNode(alias_ref, jlong nativePointer, jboolean isTextNode) { - CSSNodeSetIsTextnode(_jlong2CSSNodeRef(nativePointer), isTextNode); -} - -jboolean jni_CSSNodeGetIsTextNode(alias_ref, jlong nativePointer) { - return (jboolean) CSSNodeGetIsTextnode(_jlong2CSSNodeRef(nativePointer)); -} - jboolean jni_CSSNodeHasNewLayout(alias_ref, jlong nativePointer) { return (jboolean) CSSNodeGetHasNewLayout(_jlong2CSSNodeRef(nativePointer)); } @@ -209,8 +201,6 @@ jint JNI_OnLoad(JavaVM *vm, void *) { CSSMakeNativeMethod(jni_CSSNodeReset), CSSMakeNativeMethod(jni_CSSNodeInsertChild), CSSMakeNativeMethod(jni_CSSNodeRemoveChild), - CSSMakeNativeMethod(jni_CSSNodeSetIsTextNode), - CSSMakeNativeMethod(jni_CSSNodeGetIsTextNode), CSSMakeNativeMethod(jni_CSSNodeCalculateLayout), CSSMakeNativeMethod(jni_CSSNodeHasNewLayout), CSSMakeNativeMethod(jni_CSSNodeMarkDirty), diff --git a/tests/CSSLayoutMeasureCacheTest.cpp b/tests/CSSLayoutMeasureCacheTest.cpp index b2b90bbc..c0a8e6fa 100644 --- a/tests/CSSLayoutMeasureCacheTest.cpp +++ b/tests/CSSLayoutMeasureCacheTest.cpp @@ -59,26 +59,6 @@ TEST(CSSLayoutTest, measure_once_single_flexible_child) { CSSNodeFreeRecursive(root); } -TEST(CSSLayoutTest, remeasure_text_node_height_change) { - const CSSNodeRef root = CSSNodeNew(); - CSSNodeStyleSetAlignItems(root, CSSAlignFlexStart); - - const CSSNodeRef root_child0 = CSSNodeNew(); - CSSNodeSetIsTextnode(root_child0, true); - CSSNodeStyleSetFlexGrow(root_child0, 1); - int measureCount = 0; - CSSNodeSetContext(root_child0, &measureCount); - CSSNodeSetMeasureFunc(root_child0, _measureMax); - CSSNodeInsertChild(root, root_child0, 0); - - CSSNodeCalculateLayout(root, 100, 10, CSSDirectionLTR); - CSSNodeCalculateLayout(root, 100, 20, CSSDirectionLTR); - - ASSERT_EQ(1, measureCount); - - CSSNodeFreeRecursive(root); -} - TEST(CSSLayoutTest, remeasure_with_same_exact_width_larger_than_needed_height) { const CSSNodeRef root = CSSNodeNew();