From b59ce0910995ba9619b434b5093eb1d5b77fe68e Mon Sep 17 00:00:00 2001 From: Emil Sjolander Date: Thu, 27 Oct 2016 10:52:09 -0700 Subject: [PATCH] BREAKING - Change measure() api to remove need for MeasureOutput allocation Summary: This is an API breaking change done to allow us to avoid an allocation during measurement. Instead we do the same trick as is done when passing measure results to C, we path them into a long. Reviewed By: splhack Differential Revision: D4081037 fbshipit-source-id: 28adbcdd160cbd3f59a0fdd4b9f1200ae18678f1 --- csharp/Facebook.CSSLayout/CSSNode.cs | 5 ++--- csharp/Facebook.CSSLayout/MeasureFunction.cs | 7 ++++++- csharp/Facebook.CSSLayout/MeasureOutput.cs | 18 +++++++++++++++--- .../tests/Facebook.CSSLayout/CSSNodeTest.cs | 15 +++++++-------- java/com/facebook/csslayout/CSSNode.java | 7 ++----- java/com/facebook/csslayout/CSSNodeAPI.java | 8 +++++--- .../facebook/csslayout/CSSNodeDEPRECATED.java | 7 ++----- java/com/facebook/csslayout/LayoutEngine.java | 11 ++++++----- .../com/facebook/csslayout/MeasureOutput.java | 19 ++++++++++++++++--- .../com/facebook/csslayout/CSSNodeTest.java | 8 +++----- 10 files changed, 64 insertions(+), 41 deletions(-) diff --git a/csharp/Facebook.CSSLayout/CSSNode.cs b/csharp/Facebook.CSSLayout/CSSNode.cs index db1891f3..1e018946 100644 --- a/csharp/Facebook.CSSLayout/CSSNode.cs +++ b/csharp/Facebook.CSSLayout/CSSNode.cs @@ -543,9 +543,8 @@ namespace Facebook.CSSLayout throw new InvalidOperationException("Measure function is not defined."); } - _measureFunction(this, width, widthMode, height, heightMode, _measureOutput); - - return new CSSSize { width = _measureOutput.Width, height = _measureOutput.Height }; + long output = _measureFunction(this, width, widthMode, height, heightMode); + return new CSSSize { width = MeasureOutput.GetWidth(output), height = MeasureOutput.GetHeight(output) }; } public string Print(CSSPrintOptions options = diff --git a/csharp/Facebook.CSSLayout/MeasureFunction.cs b/csharp/Facebook.CSSLayout/MeasureFunction.cs index db4396dd..b727dfc5 100644 --- a/csharp/Facebook.CSSLayout/MeasureFunction.cs +++ b/csharp/Facebook.CSSLayout/MeasureFunction.cs @@ -9,5 +9,10 @@ namespace Facebook.CSSLayout { - public delegate void MeasureFunction(CSSNode node, float width, CSSMeasureMode widthMode, float height, CSSMeasureMode heightMode, MeasureOutput measureOutput); + public delegate long MeasureFunction( + CSSNode node, + float width, + CSSMeasureMode widthMode, + float height, + CSSMeasureMode heightMode); } diff --git a/csharp/Facebook.CSSLayout/MeasureOutput.cs b/csharp/Facebook.CSSLayout/MeasureOutput.cs index 15433937..ef3ce1d7 100644 --- a/csharp/Facebook.CSSLayout/MeasureOutput.cs +++ b/csharp/Facebook.CSSLayout/MeasureOutput.cs @@ -1,4 +1,4 @@ -/** +/** * Copyright (c) 2014-present, Facebook, Inc. * All rights reserved. * @@ -11,7 +11,19 @@ namespace Facebook.CSSLayout { public class MeasureOutput { - public float Width { get; set; } - public float Height { get; set; } + public static long Make(int width, int height) + { + return ((long) width) << 32 | ((long) height); + } + + public static int GetWidth(long measureOutput) + { + return (int) (0xFFFFFFFF & (measureOutput >> 32)); + } + + public static int GetHeight(long measureOutput) + { + return (int) (0xFFFFFFFF & measureOutput); + } } } diff --git a/csharp/tests/Facebook.CSSLayout/CSSNodeTest.cs b/csharp/tests/Facebook.CSSLayout/CSSNodeTest.cs index 640aaeed..7ebd3f96 100644 --- a/csharp/tests/Facebook.CSSLayout/CSSNodeTest.cs +++ b/csharp/tests/Facebook.CSSLayout/CSSNodeTest.cs @@ -23,18 +23,18 @@ namespace Facebook.CSSLayout { CSSNode parent = new CSSNode(); CSSNode child = new CSSNode(); - + Assert.IsNull(child.Parent); Assert.AreEqual(0, parent.Count); - + parent.Insert(0, child); - + Assert.AreEqual(1, parent.Count); Assert.AreEqual(child, parent[0]); Assert.AreEqual(parent, child.Parent); - + parent.RemoveAt(0); - + Assert.IsNull(child.Parent); Assert.AreEqual(0, parent.Count); } @@ -152,9 +152,8 @@ namespace Facebook.CSSLayout public void TestMeasureFunc() { CSSNode node = new CSSNode(); - node.SetMeasureFunction((_, width, widthMode, height, heightMode, measureResult) => { - measureResult.Width = 100; - measureResult.Height = 150; + node.SetMeasureFunction((_, width, widthMode, height, heightMode) => { + return MeasureOutput.Make(100, 150); }); node.CalculateLayout(); Assert.AreEqual(100, (int)node.LayoutWidth); diff --git a/java/com/facebook/csslayout/CSSNode.java b/java/com/facebook/csslayout/CSSNode.java index f9837b07..103784f4 100644 --- a/java/com/facebook/csslayout/CSSNode.java +++ b/java/com/facebook/csslayout/CSSNode.java @@ -487,15 +487,12 @@ public class CSSNode implements CSSNodeAPI { throw new RuntimeException("Measure function isn't defined!"); } - MeasureOutput output = new MeasureOutput(); - mMeasureFunction.measure( + return mMeasureFunction.measure( this, width, CSSMeasureMode.values()[widthMode], height, - CSSMeasureMode.values()[heightMode], - output); - return ((long) output.width) << 32 | ((long) output.height); + CSSMeasureMode.values()[heightMode]); } @Override diff --git a/java/com/facebook/csslayout/CSSNodeAPI.java b/java/com/facebook/csslayout/CSSNodeAPI.java index 95ac5718..5a5de382 100644 --- a/java/com/facebook/csslayout/CSSNodeAPI.java +++ b/java/com/facebook/csslayout/CSSNodeAPI.java @@ -12,13 +12,15 @@ package com.facebook.csslayout; public interface CSSNodeAPI { interface MeasureFunction { - void measure( + /** + * Return a value created by MeasureOutput.make(width, height); + */ + long measure( CSSNodeAPI node, float width, CSSMeasureMode widthMode, float height, - CSSMeasureMode heightMode, - MeasureOutput measureOutput); + CSSMeasureMode heightMode); } int getChildCount(); diff --git a/java/com/facebook/csslayout/CSSNodeDEPRECATED.java b/java/com/facebook/csslayout/CSSNodeDEPRECATED.java index ae7d4476..05632cf8 100644 --- a/java/com/facebook/csslayout/CSSNodeDEPRECATED.java +++ b/java/com/facebook/csslayout/CSSNodeDEPRECATED.java @@ -134,14 +134,11 @@ public class CSSNodeDEPRECATED implements CSSNodeAPI { return mIsTextNode; } - MeasureOutput measure(MeasureOutput measureOutput, float width, CSSMeasureMode widthMode, float height, CSSMeasureMode heightMode) { + long measure(float width, CSSMeasureMode widthMode, float height, CSSMeasureMode heightMode) { if (!isMeasureDefined()) { throw new RuntimeException("Measure function isn't defined!"); } - measureOutput.height = CSSConstants.UNDEFINED; - measureOutput.width = CSSConstants.UNDEFINED; - Assertions.assertNotNull(mMeasureFunction).measure(this, width, widthMode, height, heightMode, measureOutput); - return measureOutput; + return Assertions.assertNotNull(mMeasureFunction).measure(this, width, widthMode, height, heightMode); } /** diff --git a/java/com/facebook/csslayout/LayoutEngine.java b/java/com/facebook/csslayout/LayoutEngine.java index d5ae98c3..79d33bce 100644 --- a/java/com/facebook/csslayout/LayoutEngine.java +++ b/java/com/facebook/csslayout/LayoutEngine.java @@ -560,22 +560,23 @@ public class LayoutEngine { } else { // Measure the text under the current constraints. - MeasureOutput measureDim = node.measure( - - layoutContext.measureOutput, + long measureOutput = node.measure( innerWidth, widthMeasureMode, innerHeight, heightMeasureMode ); + int outputWidth = MeasureOutput.getWidth(measureOutput); + int outputHeight = MeasureOutput.getHeight(measureOutput); + node.layout.measuredDimensions[DIMENSION_WIDTH] = boundAxis(node, CSS_FLEX_DIRECTION_ROW, (widthMeasureMode == CSSMeasureMode.UNDEFINED || widthMeasureMode == CSSMeasureMode.AT_MOST) ? - measureDim.width + paddingAndBorderAxisRow : + outputWidth + paddingAndBorderAxisRow : availableWidth - marginAxisRow); node.layout.measuredDimensions[DIMENSION_HEIGHT] = boundAxis(node, CSS_FLEX_DIRECTION_COLUMN, (heightMeasureMode == CSSMeasureMode.UNDEFINED || heightMeasureMode == CSSMeasureMode.AT_MOST) ? - measureDim.height + paddingAndBorderAxisColumn : + outputHeight + paddingAndBorderAxisColumn : availableHeight - marginAxisColumn); } diff --git a/java/com/facebook/csslayout/MeasureOutput.java b/java/com/facebook/csslayout/MeasureOutput.java index 65adfdf1..5ec9fe35 100644 --- a/java/com/facebook/csslayout/MeasureOutput.java +++ b/java/com/facebook/csslayout/MeasureOutput.java @@ -10,10 +10,23 @@ package com.facebook.csslayout; /** - * POJO to hold the output of the measure function. + * Helpers for building measure output value. */ public class MeasureOutput { - public float width; - public float height; + public static long make(float width, float height) { + return make((int) width, (int) height); + } + + public static long make(int width, int height) { + return ((long) width) << 32 | ((long) height); + } + + public static int getWidth(long measureOutput) { + return (int) (0xFFFFFFFF & (measureOutput >> 32)); + } + + public static int getHeight(long measureOutput) { + return (int) (0xFFFFFFFF & measureOutput); + } } diff --git a/java/tests/com/facebook/csslayout/CSSNodeTest.java b/java/tests/com/facebook/csslayout/CSSNodeTest.java index 417567a7..c61b8988 100644 --- a/java/tests/com/facebook/csslayout/CSSNodeTest.java +++ b/java/tests/com/facebook/csslayout/CSSNodeTest.java @@ -26,15 +26,13 @@ public class CSSNodeTest { public void testMeasure() { final CSSNode node = new CSSNode(); node.setMeasureFunction(new CSSNodeAPI.MeasureFunction() { - public void measure( + public long measure( CSSNodeAPI node, float width, CSSMeasureMode widthMode, float height, - CSSMeasureMode heightMode, - MeasureOutput measureOutput) { - measureOutput.width = 100; - measureOutput.height = 100; + CSSMeasureMode heightMode) { + return MeasureOutput.make(100, 100); } }); node.calculateLayout(null);