Skip to content

Commit

Permalink
Make CompactValue internal detail of yoga::Style (facebook#1492)
Browse files Browse the repository at this point in the history 
Summary:
X-link: facebook/react-native#41776

# 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
  • Loading branch information
@NickGerleman @facebook-github-bot
NickGerleman authored and facebook-github-bot committed Dec 16, 2023
1 parent bac80ca commit a24d470
Show file tree
Hide file tree
Showing 12 changed files with 467 additions and 463 deletions.
364 changes: 151 additions & 213 deletions tests/CompactValueTest.cpp
View file

Large diffs are not rendered by default.

24 changes: 10 additions & 14 deletions yoga/YGNodeStyle.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,11 +195,7 @@ void YGNodeStyleSetFlexBasisAuto(const YGNodeRef node) {
}

YGValue YGNodeStyleGetFlexBasis(const YGNodeConstRef node) {
YGValue flexBasis = resolveRef(node)->getStyle().flexBasis();
if (flexBasis.unit == YGUnitUndefined || flexBasis.unit == YGUnitAuto) {
flexBasis.value = YGUndefined;
}
return flexBasis;
return (YGValue)resolveRef(node)->getStyle().flexBasis();
}

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

YGValue YGNodeStyleGetPosition(YGNodeConstRef node, YGEdge edge) {
return resolveRef(node)->getStyle().position(scopedEnum(edge));
return (YGValue)resolveRef(node)->getStyle().position(scopedEnum(edge));
}

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

YGValue YGNodeStyleGetMargin(YGNodeConstRef node, YGEdge edge) {
return resolveRef(node)->getStyle().margin(scopedEnum(edge));
return (YGValue)resolveRef(node)->getStyle().margin(scopedEnum(edge));
}

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

YGValue YGNodeStyleGetPadding(YGNodeConstRef node, YGEdge edge) {
return resolveRef(node)->getStyle().padding(scopedEnum(edge));
return (YGValue)resolveRef(node)->getStyle().padding(scopedEnum(edge));
}

void YGNodeStyleSetBorder(
Expand Down Expand Up @@ -309,7 +305,7 @@ void YGNodeStyleSetWidthAuto(YGNodeRef node) {
}

YGValue YGNodeStyleGetWidth(YGNodeConstRef node) {
return resolveRef(node)->getStyle().dimension(Dimension::Width);
return (YGValue)resolveRef(node)->getStyle().dimension(Dimension::Width);
}

void YGNodeStyleSetHeight(YGNodeRef node, float points) {
Expand All @@ -328,7 +324,7 @@ void YGNodeStyleSetHeightAuto(YGNodeRef node) {
}

YGValue YGNodeStyleGetHeight(YGNodeConstRef node) {
return resolveRef(node)->getStyle().dimension(Dimension::Height);
return (YGValue)resolveRef(node)->getStyle().dimension(Dimension::Height);
}

void YGNodeStyleSetMinWidth(const YGNodeRef node, const float minWidth) {
Expand All @@ -342,7 +338,7 @@ void YGNodeStyleSetMinWidthPercent(const YGNodeRef node, const float minWidth) {
}

YGValue YGNodeStyleGetMinWidth(const YGNodeConstRef node) {
return resolveRef(node)->getStyle().minDimension(Dimension::Width);
return (YGValue)resolveRef(node)->getStyle().minDimension(Dimension::Width);
}

void YGNodeStyleSetMinHeight(const YGNodeRef node, const float minHeight) {
Expand All @@ -358,7 +354,7 @@ void YGNodeStyleSetMinHeightPercent(
}

YGValue YGNodeStyleGetMinHeight(const YGNodeConstRef node) {
return resolveRef(node)->getStyle().minDimension(Dimension::Height);
return (YGValue)resolveRef(node)->getStyle().minDimension(Dimension::Height);
}

void YGNodeStyleSetMaxWidth(const YGNodeRef node, const float maxWidth) {
Expand All @@ -372,7 +368,7 @@ void YGNodeStyleSetMaxWidthPercent(const YGNodeRef node, const float maxWidth) {
}

YGValue YGNodeStyleGetMaxWidth(const YGNodeConstRef node) {
return resolveRef(node)->getStyle().maxDimension(Dimension::Width);
return (YGValue)resolveRef(node)->getStyle().maxDimension(Dimension::Width);
}

void YGNodeStyleSetMaxHeight(const YGNodeRef node, const float maxHeight) {
Expand All @@ -388,5 +384,5 @@ void YGNodeStyleSetMaxHeightPercent(
}

YGValue YGNodeStyleGetMaxHeight(const YGNodeConstRef node) {
return resolveRef(node)->getStyle().maxDimension(Dimension::Height);
return (YGValue)resolveRef(node)->getStyle().maxDimension(Dimension::Height);
}
38 changes: 20 additions & 18 deletions yoga/algorithm/CalculateLayout.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -687,9 +687,9 @@ static float distributeFreeSpaceSecondPass(
sizingModeCrossDim == SizingMode::StretchFit &&
!(isNodeFlexWrap && mainAxisOverflows) &&
resolveChildAlignment(node, currentLineChild) == Align::Stretch &&
currentLineChild->getFlexStartMarginValue(crossAxis).unit !=
YGUnitAuto &&
currentLineChild->marginTrailingValue(crossAxis).unit != YGUnitAuto) {
currentLineChild->getFlexStartMarginValue(crossAxis).unit() !=
Unit::Auto &&
currentLineChild->marginTrailingValue(crossAxis).unit() != Unit::Auto) {
childCrossSize = availableInnerCrossDim;
childCrossSizingMode = SizingMode::StretchFit;
} else if (!currentLineChild->styleDefinesDimension(
Expand All @@ -706,8 +706,8 @@ static float distributeFreeSpaceSecondPass(
.unwrap() +
marginCross;
const bool isLoosePercentageMeasurement =
currentLineChild->getResolvedDimension(dimension(crossAxis)).unit ==
YGUnitPercent &&
currentLineChild->getResolvedDimension(dimension(crossAxis)).unit() ==
Unit::Percent &&
sizingModeCrossDim != SizingMode::StretchFit;
childCrossSizingMode =
yoga::isUndefined(childCrossSize) || isLoosePercentageMeasurement
Expand All @@ -733,9 +733,9 @@ static float distributeFreeSpaceSecondPass(
const bool requiresStretchLayout = !currentLineChild->styleDefinesDimension(
crossAxis, availableInnerCrossDim) &&
resolveChildAlignment(node, currentLineChild) == Align::Stretch &&
currentLineChild->getFlexStartMarginValue(crossAxis).unit !=
YGUnitAuto &&
currentLineChild->marginTrailingValue(crossAxis).unit != YGUnitAuto;
currentLineChild->getFlexStartMarginValue(crossAxis).unit() !=
Unit::Auto &&
currentLineChild->marginTrailingValue(crossAxis).unit() != Unit::Auto;

const float childWidth = isMainAxisRow ? childMainSize : childCrossSize;
const float childHeight = !isMainAxisRow ? childMainSize : childCrossSize;
Expand Down Expand Up @@ -982,10 +982,10 @@ static void justifyMainAxis(
for (size_t i = startOfLineIndex; i < flexLine.endOfLineIndex; i++) {
auto child = node->getChild(i);
if (child->getStyle().positionType() != PositionType::Absolute) {
if (child->getFlexStartMarginValue(mainAxis).unit == YGUnitAuto) {
if (child->getFlexStartMarginValue(mainAxis).unit() == Unit::Auto) {
numberOfAutoMarginsOnCurrentLine++;
}
if (child->marginTrailingValue(mainAxis).unit == YGUnitAuto) {
if (child->marginTrailingValue(mainAxis).unit() == Unit::Auto) {
numberOfAutoMarginsOnCurrentLine++;
}
}
Expand Down Expand Up @@ -1062,7 +1062,7 @@ static void justifyMainAxis(
// We need to do that only for relative elements. Absolute elements do not
// take part in that phase.
if (childStyle.positionType() != PositionType::Absolute) {
if (child->getFlexStartMarginValue(mainAxis).unit == YGUnitAuto) {
if (child->getFlexStartMarginValue(mainAxis).unit() == Unit::Auto) {
flexLine.layout.mainDim += flexLine.layout.remainingFreeSpace /
static_cast<float>(numberOfAutoMarginsOnCurrentLine);
}
Expand All @@ -1078,7 +1078,7 @@ static void justifyMainAxis(
flexLine.layout.mainDim += betweenMainDim;
}

if (child->marginTrailingValue(mainAxis).unit == YGUnitAuto) {
if (child->marginTrailingValue(mainAxis).unit() == Unit::Auto) {
flexLine.layout.mainDim += flexLine.layout.remainingFreeSpace /
static_cast<float>(numberOfAutoMarginsOnCurrentLine);
}
Expand Down Expand Up @@ -1625,8 +1625,8 @@ static void calculateLayoutImpl(
// time, this time forcing the cross-axis size to be the computed
// cross size for the current line.
if (alignItem == Align::Stretch &&
child->getFlexStartMarginValue(crossAxis).unit != YGUnitAuto &&
child->marginTrailingValue(crossAxis).unit != YGUnitAuto) {
child->getFlexStartMarginValue(crossAxis).unit() != Unit::Auto &&
child->marginTrailingValue(crossAxis).unit() != Unit::Auto) {
// If the child defines a definite size for its cross axis, there's
// no need to stretch.
if (!child->styleDefinesDimension(
Expand Down Expand Up @@ -1699,15 +1699,17 @@ static void calculateLayoutImpl(
const float remainingCrossDim = containerCrossAxis -
child->dimensionWithMargin(crossAxis, availableInnerWidth);

if (child->getFlexStartMarginValue(crossAxis).unit == YGUnitAuto &&
child->marginTrailingValue(crossAxis).unit == YGUnitAuto) {
if (child->getFlexStartMarginValue(crossAxis).unit() ==
Unit::Auto &&
child->marginTrailingValue(crossAxis).unit() == Unit::Auto) {
leadingCrossDim +=
yoga::maxOrDefined(0.0f, remainingCrossDim / 2);
} else if (
child->marginTrailingValue(crossAxis).unit == YGUnitAuto) {
child->marginTrailingValue(crossAxis).unit() == Unit::Auto) {
// No-Op
} else if (
child->getFlexStartMarginValue(crossAxis).unit == YGUnitAuto) {
child->getFlexStartMarginValue(crossAxis).unit() ==
Unit::Auto) {
leadingCrossDim += yoga::maxOrDefined(0.0f, remainingCrossDim);
} else if (alignItem == Align::FlexStart) {
// No-Op
Expand Down
16 changes: 6 additions & 10 deletions yoga/algorithm/ResolveValue.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,15 @@

namespace facebook::yoga {

inline FloatOptional resolveValue(const YGValue value, const float ownerSize) {
switch (value.unit) {
case YGUnitPoint:
return FloatOptional{value.value};
case YGUnitPercent:
return FloatOptional{value.value * ownerSize * 0.01f};
inline FloatOptional resolveValue(Style::Length length, float ownerSize) {
switch (length.unit()) {
case Unit::Point:
return length.value();
case Unit::Percent:
return FloatOptional{length.value().unwrap() * ownerSize * 0.01f};
default:
return FloatOptional{};
}
}

inline FloatOptional resolveValue(Style::Length value, float ownerSize) {
return resolveValue((YGValue)value, ownerSize);
}

} // namespace facebook::yoga
24 changes: 14 additions & 10 deletions yoga/debug/NodeToString.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,34 +46,38 @@ static void appendFloatOptionalIfDefined(
static void appendNumberIfNotUndefined(
std::string& base,
const std::string key,
const YGValue number) {
if (number.unit != YGUnitUndefined) {
if (number.unit == YGUnitAuto) {
const Style::Length& number) {
if (number.unit() != Unit::Undefined) {
if (number.unit() == Unit::Auto) {
base.append(key + ": auto; ");
} else {
std::string unit = number.unit == YGUnitPoint ? "px" : "%%";
std::string unit = number.unit() == Unit::Point ? "px" : "%%";
appendFormattedString(
base, "%s: %g%s; ", key.c_str(), number.value, unit.c_str());
base,
"%s: %g%s; ",
key.c_str(),
number.value().unwrap(),
unit.c_str());
}
}
}

static void appendNumberIfNotAuto(
std::string& base,
const std::string& key,
const YGValue number) {
if (number.unit != YGUnitAuto) {
const Style::Length& number) {
if (number.unit() != Unit::Auto) {
appendNumberIfNotUndefined(base, key, number);
}
}

static void appendNumberIfNotZero(
std::string& base,
const std::string& str,
const YGValue number) {
if (number.unit == YGUnitAuto) {
const Style::Length& number) {
if (number.unit() == Unit::Auto) {
base.append(str + ": auto; ");
} else if (!yoga::inexactEquals(number.value, 0)) {
} else if (!yoga::inexactEquals(number.value().unwrap(), 0)) {
appendNumberIfNotUndefined(base, str, number);
}
}
Expand Down
Loading

0 comments on commit a24d470

Please sign in to comment.