Repository navigation
Keep Yoga's min-content pass off committed nodes - #59688
YevheniiKotyrlo wants to merge 1 commit into
Conversation
computeMinContentMainSize walks a flex item's subtree through getChild(i) and measures the leaves it reaches without cloning them first. An item's children are still the committed tree's until the item's own layout clones them, whether its measurement comes from its cache or its flex basis from its style, so the pass measures, and memoises into, nodes of the committed tree. A committed Paragraph there keeps content that points at the inline View its own layout replaced, which is freed by then: React Native crashes in ParagraphShadowNode::measureContent. The pass now clones a container's children before descending, as calculateLayoutImpl and layoutAbsoluteDescendants do before writing to them. So that it does not clone every item's subtree on every layout, a memoised min-content now holds as long as the node's cached measurements do: calculateLayoutInternal drops it with them when it lays out a changed node, and a later layout walks only the subtrees that changed. A container's memo is keyed on the owner's width only when a percentage margin or padding in the subtree it walked resolves against it, as a cached layout is since react/yoga#2042, so a new width alone does not walk an item whose layout comes from its cache. Since a memo now outlives a layout, YGNodeSetMinContentWidth and YGNodeSetMinContentHeight mark the node dirty when the value changes, as style setters do, and a min-content measure function's results are kept, as a measure function's are, until the node is marked dirty. React Native's measure connector asserts that the node it measures is not sealed, and Fantom registers LayoutConformance, so a test can lay out under strict conformance.
Cost of a relayout: instructions per commit,
|
| Rows, and what each commit changes | main |
this PR |
|---|---|---|
| 100 rows, a View below them changes | 2,869,119 | 2,349,628 (−18.1%) |
| 1,000 rows, a View below them changes | 28,179,469 | 22,987,178 (−18.4%) |
| 100 rows, the column's width changes | 2,515,926 | 1,995,839 (−20.7%) |
| 1,000 rows, the column's width changes | 24,680,834 | 19,482,547 (−21.1%) |
Each count is callgrind's over the four steps the driver runs per commit — ShadowNode::cloneTree (RootShadowNode::clone for the width), RootShadowNode::layoutIfNeeded, sealRecursive and calculateShadowViewMutations — in a Release build of the renderer by Ubuntu clang 14 in the reactnativecommunity/react-native-android image, against the libraries Fantom's tester links, so text is measured by the C++ platform's TextLayoutManager, which lays no text out. Per commit = (count at 12 commits − count at 2) / 10, and the chart divides it by the rows. Both builds emit the same number of mutations. The count is deterministic: main built and counted again in a separate run gives the same numbers to the instruction, so there is no spread or noise floor to report.
How to rerun
After yarn fantom has built the tester once, add the driver below to private/react-native-fantom/tester as a target that links what fantom_tester links, configure that project in Release as Fantom's build does, and count each shape on main and on this branch:
get_target_property(FANTOM_LIBRARIES fantom_tester LINK_LIBRARIES)
add_executable(min_content_commit_instructions min-content-commit-instructions.cpp)
target_include_directories(min_content_commit_instructions PRIVATE ${REACT_COMMON_DIR})
target_link_libraries(min_content_commit_instructions ${FANTOM_LIBRARIES})
target_compile_options(min_content_commit_instructions PRIVATE -fexceptions -frtti -std=c++20)for shape in footer-100 footer-1000 width-100 width-1000; do
for commits in 2 12; do
valgrind --tool=callgrind --collect-atstart=no '--toggle-collect=*commitRepeatedly*' \
./min_content_commit_instructions "$shape" "$commits"
done
doneDriver
// Commits under strict layout conformance with enableFlexboxAutoMinSizeInStrictMode on that lay a column of rows, each
// a View holding a Text, out again while every row stays as the last commit left it. A `footer` commit changes the
// height of a View below the rows; a `width` commit changes the column's width, its rows keeping a fixed width. Each
// commit is ShadowNode::cloneTree (or RootShadowNode::clone for the width), RootShadowNode::layoutIfNeeded,
// sealRecursive and calculateShadowViewMutations.
// min-content-commit-instructions <footer|width>-<rows> <commits>
#include <cstdio>
#include <cstdlib>
#include <memory>
#include <string>
#include <vector>
#include <react/featureflags/ReactNativeFeatureFlags.h>
#include <react/featureflags/ReactNativeFeatureFlagsDefaults.h>
#include <react/renderer/componentregistry/ComponentDescriptorProviderRegistry.h>
#include <react/renderer/components/root/RootComponentDescriptor.h>
#include <react/renderer/components/text/ParagraphComponentDescriptor.h>
#include <react/renderer/components/text/RawTextComponentDescriptor.h>
#include <react/renderer/components/view/LayoutConformanceComponentDescriptor.h>
#include <react/renderer/components/view/ViewComponentDescriptor.h>
#include <react/renderer/core/PropsParserContext.h>
#include <react/renderer/element/ComponentBuilder.h>
#include <react/renderer/element/Element.h>
#include <react/renderer/mounting/Differentiator.h>
#include <react/utils/ContextContainer.h>
// The tester links no element library, so the builder's one source is compiled here.
#include <react/renderer/element/ComponentBuilder.cpp>
using namespace facebook::react;
namespace yoga = facebook::yoga;
namespace {
enum class Change { Footer, Width };
class AutoMinSizeFlags final : public ReactNativeFeatureFlagsDefaults {
public:
bool enableFlexboxAutoMinSizeInStrictMode() override {
return true;
}
};
ComponentBuilder makeBuilder() {
ComponentDescriptorProviderRegistry providers{};
auto registry = providers.createComponentDescriptorRegistry(ComponentDescriptorParameters{
.eventDispatcher = EventDispatcher::Shared{}, .contextContainer = nullptr, .flavor = nullptr});
providers.add(concreteComponentDescriptorProvider<RootComponentDescriptor>());
providers.add(concreteComponentDescriptorProvider<ViewComponentDescriptor>());
providers.add(concreteComponentDescriptorProvider<LayoutConformanceComponentDescriptor>());
providers.add(concreteComponentDescriptorProvider<ParagraphComponentDescriptor>());
providers.add(concreteComponentDescriptorProvider<RawTextComponentDescriptor>());
return ComponentBuilder{registry};
}
std::shared_ptr<const ViewProps> footerProps(float height) {
auto props = std::make_shared<ViewProps>();
props->yogaStyle.setDimension(yoga::Dimension::Height, yoga::StyleSizeLength::points(height));
return props;
}
std::shared_ptr<const ViewProps> rowProps(Change change) {
auto props = std::make_shared<ViewProps>();
switch (change) {
case Change::Footer:
break;
case Change::Width:
props->yogaStyle.setDimension(yoga::Dimension::Width, yoga::StyleSizeLength::points(200));
break;
}
return props;
}
std::shared_ptr<const ViewProps> columnProps() {
auto props = std::make_shared<ViewProps>();
props->yogaStyle.setFlex(yoga::FloatOptional{1.0f});
return props;
}
std::shared_ptr<const RawTextProps> rawTextProps(int row) {
auto props = std::make_shared<RawTextProps>();
props->text = "Row " + std::to_string(row) + " ";
return props;
}
LayoutConstraints rootConstraints(float width) {
return LayoutConstraints{
.minimumSize = {.width = width, .height = 100000}, .maximumSize = {.width = width, .height = 100000}};
}
struct Tree {
std::shared_ptr<RootShadowNode> root;
std::shared_ptr<ViewShadowNode> footer;
};
Tree buildTree(const ComponentBuilder& builder, Change change, int rows) {
Tag tag = 2;
std::vector<ElementFragment> children;
Tree tree;
for (int row = 0; row < rows; row++) {
children.push_back(Element<ViewShadowNode>().tag(tag++).surfaceId(1).props(rowProps(change)).children(
{Element<ParagraphShadowNode>().tag(tag++).surfaceId(1).children(
{Element<RawTextShadowNode>().tag(tag++).surfaceId(1).props(rawTextProps(row))})}));
}
children.push_back(Element<ViewShadowNode>().tag(tag++).surfaceId(1).props(footerProps(10)).reference(tree.footer));
auto conformanceProps = std::make_shared<LayoutConformanceProps>();
conformanceProps->mode = LayoutConformance::Strict;
conformanceProps->yogaStyle.setDisplay(yoga::Display::Contents);
auto rootProps = std::make_shared<RootProps>();
rootProps->layoutConstraints = rootConstraints(400);
tree.root = builder.build(
Element<RootShadowNode>().tag(1).surfaceId(1).props(rootProps).children(
{Element<LayoutConformanceShadowNode>().tag(tag++).surfaceId(1).props(conformanceProps).children(
{Element<ViewShadowNode>().tag(tag++).surfaceId(1).props(columnProps()).children(children)})}));
tree.root->layoutIfNeeded();
tree.root->sealRecursive();
return tree;
}
const ShadowNode* firstParagraph(const RootShadowNode& root) {
const auto& column = root.getChildren().at(0)->getChildren().at(0);
return column->getChildren().at(0)->getChildren().at(0).get();
}
[[gnu::noinline]] size_t commitRepeatedly(
std::shared_ptr<RootShadowNode>& root,
const ShadowNodeFamily& footerFamily,
const PropsParserContext& parserContext,
Change change,
int commits) {
size_t mutations = 0;
for (int commit = 0; commit < commits; commit++) {
const bool even = commit % 2 == 0;
std::shared_ptr<RootShadowNode> newRoot;
switch (change) {
case Change::Footer: {
const auto props = footerProps(even ? 20 : 10);
newRoot = std::static_pointer_cast<RootShadowNode>(root->cloneTree(
footerFamily, [&](const ShadowNode& oldShadowNode) { return oldShadowNode.clone({.props = props}); }));
break;
}
case Change::Width:
newRoot = root->clone(parserContext, rootConstraints(even ? 390 : 400), LayoutContext{});
break;
}
newRoot->layoutIfNeeded();
newRoot->sealRecursive();
mutations += calculateShadowViewMutations(*root, *newRoot).size();
root = newRoot;
}
return mutations;
}
} // namespace
int main(int argumentCount, char** arguments) {
const std::string shape = argumentCount == 3 ? arguments[1] : "";
const auto separator = shape.find('-');
const std::string changed = shape.substr(0, separator);
if (separator == std::string::npos || (changed != "footer" && changed != "width")) {
std::fprintf(stderr, "usage: min-content-commit-instructions <footer|width>-<rows> <commits>\n");
return 2;
}
const Change change = changed == "width" ? Change::Width : Change::Footer;
const int rows = std::atoi(shape.substr(separator + 1).c_str());
const int commits = std::atoi(arguments[2]);
ReactNativeFeatureFlags::override(std::make_unique<AutoMinSizeFlags>());
const auto builder = makeBuilder();
auto tree = buildTree(builder, change, rows);
const auto* paragraphBefore = firstParagraph(*tree.root);
auto root = tree.root;
const ContextContainer contextContainer;
const PropsParserContext parserContext{1, contextContainer};
const auto mutations = commitRepeatedly(root, tree.footer->getFamily(), parserContext, change, commits);
std::fprintf(stderr, "%zu mutations\n", mutations);
std::printf(
"%s %d commits, first paragraph %s\n",
shape.c_str(),
commits,
firstParagraph(*root) == paragraphBefore ? "shared" : "cloned");
return 0;
}
Summary:
Fixes #59678. With
enableFlexboxAutoMinSizeInStrictModeon, a Text that holds a View under<experimental_LayoutConformance mode="strict">crashes on the JavaScript thread once a later commit lays its container out again while the Text stays as an earlier commit laid it out.computeMinContentMainSize(#57057, memoised in #58833) walks each flex item's subtree throughgetChild(i)and measures the leaves it reaches, but unlike the rest of the algorithm it never callscloneChildrenIfNeeded(). An item's children are still the committed tree's until the item's own layout clones them, whether its measurement then comes from its cache or its flex basis from its style, so the pass runs measure callbacks on, and writes its memoised results into, nodes the tree being laid out does not own. A committed Paragraph's cached content still points at the View its ownlayout()replaced with a laid-out clone, and React's reference followed that clone, so the View is already destroyed when the pass reads it.This clones a container's children before the pass descends into them, as
calculateLayoutImpldoes before its writes andlayoutAbsoluteDescendantsdoes since react/yoga#1675. So that a relayout does not clone every item's subtree, a memoised min-content now holds as long as the node's cached measurements do: the pass reuses it under the conditioncalculateLayoutInternalreuses them, nowisCacheStale, andcalculateLayoutInternaldrops a memo from an earlier layout where it drops them. A container's memo is keyed on the owner's width only when a percentage margin or padding in the subtree it walked resolves against it, as a cached layout is since react/yoga#2042. A relayout then walks, and clones, only the subtrees that changed.Because a memo now outlives its layout,
YGNodeSetMinContentWidthandYGNodeSetMinContentHeightmark the node dirty when the value changes, as style setters do, andYGMinContentMeasureFuncsays its results are kept, as a measure function's are, until the node is marked dirty.yogaNodeMeasureCallbackConnectornow callsensureUnsealed()on the node it measures, so a committed node reaching it asserts in debug builds where it happens rather than as a read of freed memory later, and Fantom registersLayoutConformance, which no test could lay out under before.Changelog:
[GENERAL] [FIXED] - Fix a crash measuring a Text that holds a View under strict layout conformance with automatic minimum sizes
Test Plan:
yarn fantom Text-flexboxAutoMinSize-itest View-flexboxAutoMinSize-itest: onmainwith only the tests and the assertion, the Text file aborts withAttempt to mutate a sealed object.and the View cases pass; with this change all four cases pass.MinContentTest.cpp, which I built with gtest against the libraries Fantom's tester links: onmain,testWalkClonesChildrenOfAnotherTreeandtestUnchangedItemIsNotMeasuredAgainfail; with this change all five cases pass.yoga/matches this base's, pass with this change applied.mainwhen a View below them changes, and 21% fewer when the column's width changes (counts).lint,test_js,build_js_types,format,validate_cxx_api_snapshots,fantomandbuild_androidpass in React Native's Android CI image. I could not runtest_ios_rntester; Yoga's sources compile for the iOS simulator withYoga.podspec's flags.