Skip to content

Commit 192016a

Browse files
NickGerlemanfacebook-github-bot
NickGerleman
authored and
facebook-github-bot
committed
Make CompactValue internal detail of yoga::Style (#1492)
Summary: X-link: facebook/react-native#41776 Pull Request resolved: #1492 # Summary In preparation to replace `CompactValue`, this fully encapsulates it as an implementation detail of `yoga::Style`. The internal API now always operates on `Style::Length`, converted to `YGValue` at the public API boundary. In the next step, we can plug in a new representation within `Style`, which should enable 64 bit values, and lower memory usage. # Test Plan 1. Existing tests (inc for style, invalidation, CompactValue) pass 2. Check that constexpr `yoga::isinf()` produces same assembly under Clang as `std::isinf()` 3. Fabric Android builds 4. Yoga benchmark does style reads # Performance Checking whether a style is defined, then reading after, is a hot path, and we are doubling any space style lengths take in the stack (but not long-term on the node). After a naive move, on one system, the Yoga benchmark creating, laying out, and destroying a tree, ran about 8-10% slower in the "Huge nested flex" example. We are converting in many more cases instead of doing undefined check, but operating on accessed style values no longer needs to do the conversion multiple times. I changed the `CompactValue` conversion to YGValue/StyleLength path to check for undefined as the common case (since we always convert, instead of calling `isUndefined` directly on CompactValue. That seemed to get the difference down to ~5-6% when I was playing with it then. We can optimistically make some of this up with ValuePool giving better locality, and fix this more holistically if we reduce edge and value resolution. On another machine where I tested this, the new revision went the opposite direction, and was about 5% faster, so this isn't really a cut and dry regression, but we see different characteristics than before. # Changelog [Internal] Reviewed By: rozele Differential Revision: D51775346 fbshipit-source-id: c618af41b4882b4a227c917fcad07375806faf78
1 parent 2caa8ac commit 192016a

12 files changed

+466
-462
lines changed

‎tests/CompactValueTest.cpp

+151-213
Large diffs are not rendered by default.

‎yoga/YGNodeStyle.cpp

+10-14
Original file line numberDiff line numberDiff line change
@@ -195,11 +195,7 @@ void YGNodeStyleSetFlexBasisAuto(const YGNodeRef node) {
195195
}
196196

197197
YGValue YGNodeStyleGetFlexBasis(const YGNodeConstRef node) {
198-
YGValue flexBasis = resolveRef(node)->getStyle().flexBasis();
199-
if (flexBasis.unit == YGUnitUndefined || flexBasis.unit == YGUnitAuto) {
200-
flexBasis.value = YGUndefined;
201-
}
202-
return flexBasis;
198+
return (YGValue)resolveRef(node)->getStyle().flexBasis();
203199
}
204200

205201
void YGNodeStyleSetPosition(YGNodeRef node, YGEdge edge, float points) {
@@ -213,7 +209,7 @@ void YGNodeStyleSetPositionPercent(YGNodeRef node, YGEdge edge, float percent) {
213209
}
214210

215211
YGValue YGNodeStyleGetPosition(YGNodeConstRef node, YGEdge edge) {
216-
return resolveRef(node)->getStyle().position(scopedEnum(edge));
212+
return (YGValue)resolveRef(node)->getStyle().position(scopedEnum(edge));
217213
}
218214

219215
void YGNodeStyleSetMargin(YGNodeRef node, YGEdge edge, float points) {
@@ -232,7 +228,7 @@ void YGNodeStyleSetMarginAuto(YGNodeRef node, YGEdge edge) {
232228
}
233229

234230
YGValue YGNodeStyleGetMargin(YGNodeConstRef node, YGEdge edge) {
235-
return resolveRef(node)->getStyle().margin(scopedEnum(edge));
231+
return (YGValue)resolveRef(node)->getStyle().margin(scopedEnum(edge));
236232
}
237233

238234
void YGNodeStyleSetPadding(YGNodeRef node, YGEdge edge, float points) {
@@ -246,7 +242,7 @@ void YGNodeStyleSetPaddingPercent(YGNodeRef node, YGEdge edge, float percent) {
246242
}
247243

248244
YGValue YGNodeStyleGetPadding(YGNodeConstRef node, YGEdge edge) {
249-
return resolveRef(node)->getStyle().padding(scopedEnum(edge));
245+
return (YGValue)resolveRef(node)->getStyle().padding(scopedEnum(edge));
250246
}
251247

252248
void YGNodeStyleSetBorder(
@@ -309,7 +305,7 @@ void YGNodeStyleSetWidthAuto(YGNodeRef node) {
309305
}
310306

311307
YGValue YGNodeStyleGetWidth(YGNodeConstRef node) {
312-
return resolveRef(node)->getStyle().dimension(Dimension::Width);
308+
return (YGValue)resolveRef(node)->getStyle().dimension(Dimension::Width);
313309
}
314310

315311
void YGNodeStyleSetHeight(YGNodeRef node, float points) {
@@ -328,7 +324,7 @@ void YGNodeStyleSetHeightAuto(YGNodeRef node) {
328324
}
329325

330326
YGValue YGNodeStyleGetHeight(YGNodeConstRef node) {
331-
return resolveRef(node)->getStyle().dimension(Dimension::Height);
327+
return (YGValue)resolveRef(node)->getStyle().dimension(Dimension::Height);
332328
}
333329

334330
void YGNodeStyleSetMinWidth(const YGNodeRef node, const float minWidth) {
@@ -342,7 +338,7 @@ void YGNodeStyleSetMinWidthPercent(const YGNodeRef node, const float minWidth) {
342338
}
343339

344340
YGValue YGNodeStyleGetMinWidth(const YGNodeConstRef node) {
345-
return resolveRef(node)->getStyle().minDimension(Dimension::Width);
341+
return (YGValue)resolveRef(node)->getStyle().minDimension(Dimension::Width);
346342
}
347343

348344
void YGNodeStyleSetMinHeight(const YGNodeRef node, const float minHeight) {
@@ -358,7 +354,7 @@ void YGNodeStyleSetMinHeightPercent(
358354
}
359355

360356
YGValue YGNodeStyleGetMinHeight(const YGNodeConstRef node) {
361-
return resolveRef(node)->getStyle().minDimension(Dimension::Height);
357+
return (YGValue)resolveRef(node)->getStyle().minDimension(Dimension::Height);
362358
}
363359

364360
void YGNodeStyleSetMaxWidth(const YGNodeRef node, const float maxWidth) {
@@ -372,7 +368,7 @@ void YGNodeStyleSetMaxWidthPercent(const YGNodeRef node, const float maxWidth) {
372368
}
373369

374370
YGValue YGNodeStyleGetMaxWidth(const YGNodeConstRef node) {
375-
return resolveRef(node)->getStyle().maxDimension(Dimension::Width);
371+
return (YGValue)resolveRef(node)->getStyle().maxDimension(Dimension::Width);
376372
}
377373

378374
void YGNodeStyleSetMaxHeight(const YGNodeRef node, const float maxHeight) {
@@ -388,5 +384,5 @@ void YGNodeStyleSetMaxHeightPercent(
388384
}
389385

390386
YGValue YGNodeStyleGetMaxHeight(const YGNodeConstRef node) {
391-
return resolveRef(node)->getStyle().maxDimension(Dimension::Height);
387+
return (YGValue)resolveRef(node)->getStyle().maxDimension(Dimension::Height);
392388
}

‎yoga/algorithm/CalculateLayout.cpp

+20-18
Original file line numberDiff line numberDiff line change
@@ -687,9 +687,9 @@ static float distributeFreeSpaceSecondPass(
687687
sizingModeCrossDim == SizingMode::StretchFit &&
688688
!(isNodeFlexWrap && mainAxisOverflows) &&
689689
resolveChildAlignment(node, currentLineChild) == Align::Stretch &&
690-
currentLineChild->getFlexStartMarginValue(crossAxis).unit !=
691-
YGUnitAuto &&
692-
currentLineChild->marginTrailingValue(crossAxis).unit != YGUnitAuto) {
690+
currentLineChild->getFlexStartMarginValue(crossAxis).unit() !=
691+
Unit::Auto &&
692+
currentLineChild->marginTrailingValue(crossAxis).unit() != Unit::Auto) {
693693
childCrossSize = availableInnerCrossDim;
694694
childCrossSizingMode = SizingMode::StretchFit;
695695
} else if (!currentLineChild->styleDefinesDimension(
@@ -706,8 +706,8 @@ static float distributeFreeSpaceSecondPass(
706706
.unwrap() +
707707
marginCross;
708708
const bool isLoosePercentageMeasurement =
709-
currentLineChild->getResolvedDimension(dimension(crossAxis)).unit ==
710-
YGUnitPercent &&
709+
currentLineChild->getResolvedDimension(dimension(crossAxis)).unit() ==
710+
Unit::Percent &&
711711
sizingModeCrossDim != SizingMode::StretchFit;
712712
childCrossSizingMode =
713713
yoga::isUndefined(childCrossSize) || isLoosePercentageMeasurement
@@ -733,9 +733,9 @@ static float distributeFreeSpaceSecondPass(
733733
const bool requiresStretchLayout = !currentLineChild->styleDefinesDimension(
734734
crossAxis, availableInnerCrossDim) &&
735735
resolveChildAlignment(node, currentLineChild) == Align::Stretch &&
736-
currentLineChild->getFlexStartMarginValue(crossAxis).unit !=
737-
YGUnitAuto &&
738-
currentLineChild->marginTrailingValue(crossAxis).unit != YGUnitAuto;
736+
currentLineChild->getFlexStartMarginValue(crossAxis).unit() !=
737+
Unit::Auto &&
738+
currentLineChild->marginTrailingValue(crossAxis).unit() != Unit::Auto;
739739

740740
const float childWidth = isMainAxisRow ? childMainSize : childCrossSize;
741741
const float childHeight = !isMainAxisRow ? childMainSize : childCrossSize;
@@ -982,10 +982,10 @@ static void justifyMainAxis(
982982
for (size_t i = startOfLineIndex; i < flexLine.endOfLineIndex; i++) {
983983
auto child = node->getChild(i);
984984
if (child->getStyle().positionType() != PositionType::Absolute) {
985-
if (child->getFlexStartMarginValue(mainAxis).unit == YGUnitAuto) {
985+
if (child->getFlexStartMarginValue(mainAxis).unit() == Unit::Auto) {
986986
numberOfAutoMarginsOnCurrentLine++;
987987
}
988-
if (child->marginTrailingValue(mainAxis).unit == YGUnitAuto) {
988+
if (child->marginTrailingValue(mainAxis).unit() == Unit::Auto) {
989989
numberOfAutoMarginsOnCurrentLine++;
990990
}
991991
}
@@ -1062,7 +1062,7 @@ static void justifyMainAxis(
10621062
// We need to do that only for relative elements. Absolute elements do not
10631063
// take part in that phase.
10641064
if (childStyle.positionType() != PositionType::Absolute) {
1065-
if (child->getFlexStartMarginValue(mainAxis).unit == YGUnitAuto) {
1065+
if (child->getFlexStartMarginValue(mainAxis).unit() == Unit::Auto) {
10661066
flexLine.layout.mainDim += flexLine.layout.remainingFreeSpace /
10671067
static_cast<float>(numberOfAutoMarginsOnCurrentLine);
10681068
}
@@ -1078,7 +1078,7 @@ static void justifyMainAxis(
10781078
flexLine.layout.mainDim += betweenMainDim;
10791079
}
10801080

1081-
if (child->marginTrailingValue(mainAxis).unit == YGUnitAuto) {
1081+
if (child->marginTrailingValue(mainAxis).unit() == Unit::Auto) {
10821082
flexLine.layout.mainDim += flexLine.layout.remainingFreeSpace /
10831083
static_cast<float>(numberOfAutoMarginsOnCurrentLine);
10841084
}
@@ -1630,8 +1630,8 @@ static void calculateLayoutImpl(
16301630
// time, this time forcing the cross-axis size to be the computed
16311631
// cross size for the current line.
16321632
if (alignItem == Align::Stretch &&
1633-
child->getFlexStartMarginValue(crossAxis).unit != YGUnitAuto &&
1634-
child->marginTrailingValue(crossAxis).unit != YGUnitAuto) {
1633+
child->getFlexStartMarginValue(crossAxis).unit() != Unit::Auto &&
1634+
child->marginTrailingValue(crossAxis).unit() != Unit::Auto) {
16351635
// If the child defines a definite size for its cross axis, there's
16361636
// no need to stretch.
16371637
if (!child->styleDefinesDimension(
@@ -1704,15 +1704,17 @@ static void calculateLayoutImpl(
17041704
const float remainingCrossDim = containerCrossAxis -
17051705
child->dimensionWithMargin(crossAxis, availableInnerWidth);
17061706

1707-
if (child->getFlexStartMarginValue(crossAxis).unit == YGUnitAuto &&
1708-
child->marginTrailingValue(crossAxis).unit == YGUnitAuto) {
1707+
if (child->getFlexStartMarginValue(crossAxis).unit() ==
1708+
Unit::Auto &&
1709+
child->marginTrailingValue(crossAxis).unit() == Unit::Auto) {
17091710
leadingCrossDim +=
17101711
yoga::maxOrDefined(0.0f, remainingCrossDim / 2);
17111712
} else if (
1712-
child->marginTrailingValue(crossAxis).unit == YGUnitAuto) {
1713+
child->marginTrailingValue(crossAxis).unit() == Unit::Auto) {
17131714
// No-Op
17141715
} else if (
1715-
child->getFlexStartMarginValue(crossAxis).unit == YGUnitAuto) {
1716+
child->getFlexStartMarginValue(crossAxis).unit() ==
1717+
Unit::Auto) {
17161718
leadingCrossDim += yoga::maxOrDefined(0.0f, remainingCrossDim);
17171719
} else if (alignItem == Align::FlexStart) {
17181720
// No-Op

‎yoga/algorithm/ResolveValue.h

+6-10
Original file line numberDiff line numberDiff line change
@@ -14,19 +14,15 @@
1414

1515
namespace facebook::yoga {
1616

17-
inline FloatOptional resolveValue(const YGValue value, const float ownerSize) {
18-
switch (value.unit) {
19-
case YGUnitPoint:
20-
return FloatOptional{value.value};
21-
case YGUnitPercent:
22-
return FloatOptional{value.value * ownerSize * 0.01f};
17+
inline FloatOptional resolveValue(Style::Length length, float ownerSize) {
18+
switch (length.unit()) {
19+
case Unit::Point:
20+
return length.value();
21+
case Unit::Percent:
22+
return FloatOptional{length.value().unwrap() * ownerSize * 0.01f};
2323
default:
2424
return FloatOptional{};
2525
}
2626
}
2727

28-
inline FloatOptional resolveValue(Style::Length value, float ownerSize) {
29-
return resolveValue((YGValue)value, ownerSize);
30-
}
31-
3228
} // namespace facebook::yoga

‎yoga/debug/NodeToString.cpp

+14-10
Original file line numberDiff line numberDiff line change
@@ -46,34 +46,38 @@ static void appendFloatOptionalIfDefined(
4646
static void appendNumberIfNotUndefined(
4747
std::string& base,
4848
const std::string key,
49-
const YGValue number) {
50-
if (number.unit != YGUnitUndefined) {
51-
if (number.unit == YGUnitAuto) {
49+
const Style::Length& number) {
50+
if (number.unit() != Unit::Undefined) {
51+
if (number.unit() == Unit::Auto) {
5252
base.append(key + ": auto; ");
5353
} else {
54-
std::string unit = number.unit == YGUnitPoint ? "px" : "%%";
54+
std::string unit = number.unit() == Unit::Point ? "px" : "%%";
5555
appendFormattedString(
56-
base, "%s: %g%s; ", key.c_str(), number.value, unit.c_str());
56+
base,
57+
"%s: %g%s; ",
58+
key.c_str(),
59+
number.value().unwrap(),
60+
unit.c_str());
5761
}
5862
}
5963
}
6064

6165
static void appendNumberIfNotAuto(
6266
std::string& base,
6367
const std::string& key,
64-
const YGValue number) {
65-
if (number.unit != YGUnitAuto) {
68+
const Style::Length& number) {
69+
if (number.unit() != Unit::Auto) {
6670
appendNumberIfNotUndefined(base, key, number);
6771
}
6872
}
6973

7074
static void appendNumberIfNotZero(
7175
std::string& base,
7276
const std::string& str,
73-
const YGValue number) {
74-
if (number.unit == YGUnitAuto) {
77+
const Style::Length& number) {
78+
if (number.unit() == Unit::Auto) {
7579
base.append(str + ": auto; ");
76-
} else if (!yoga::inexactEquals(number.value, 0)) {
80+
} else if (!yoga::inexactEquals(number.value().unwrap(), 0)) {
7781
appendNumberIfNotUndefined(base, str, number);
7882
}
7983
}

0 commit comments

Comments
 (0)