Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Makes Yoga threadsafed: #791

Closed
wants to merge 2 commits into from
Closed

Makes Yoga threadsafed: #791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3fa3c12
Makes Yoga threadsafed:
Jun 19, 2018
977b74a
Merge branch 'master' into yoga_thread_safety_master
rockerhieu Jul 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions yoga/YGNode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,14 +121,17 @@ void YGNode::setMeasureFunc(YGMeasureFunc measureFunc) {

void YGNode::replaceChild(YGNodeRef child, uint32_t index) {
children_[index] = child;
setChildRoot(child);
}

void YGNode::replaceChild(YGNodeRef oldChild, YGNodeRef newChild) {
std::replace(children_.begin(), children_.end(), oldChild, newChild);
setChildRoot(newChild);
}

void YGNode::insertChild(YGNodeRef child, uint32_t index) {
children_.insert(children_.begin() + index, child);
setChildRoot(child);
}

void YGNode::setDirty(bool isDirty) {
Expand Down Expand Up @@ -552,3 +555,15 @@ bool YGNode::isLayoutTreeEqualToNode(const YGNode& node) const {
}
return isLayoutTreeEqual;
}

YGNodeRef YGNode::getRoot() {
if (pRoot == nullptr) pRoot = this;
return pRoot;
}

void YGNode::setChildRoot(YGNodeRef node) {
node->pRoot = getRoot();
for (auto child: node->children_) {
setChildRoot(child);
}
}
9 changes: 9 additions & 0 deletions yoga/YGNode.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,4 +270,13 @@ struct YGNode {
bool isNodeFlexible();
bool didUseLegacyFlag();
bool isLayoutTreeEqualToNode(const YGNode& node) const;

private:
YGNodeRef pRoot = nullptr;

public:
uint32_t gCurrentGenerationCount = 0;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably reduce the storage here to just 16-bits (uint16_t), saving 2 bytes overhead per node here, and in YGLayout (saving another 4 bytes). Do we really need to track more than 65,535 generations?

Since this is now a member, we probably shouldn't use the "g" (global) prefix. It might also be a good idea to make it private, with inlined access via:

  • void incrementGeneration() { this.generation++ }
  • bool isEqualToGeneration(uint16_t g) { return g == this.generation }

uint32_t gDepth = 0;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can avoid storing gDepth in every node (4 bytes overhead) by:

  1. Passing gDepth (perhaps renamed to just depth, again avoiding the global naming prefix) between function calls that stem from YGNodeCalculateLayout.
  2. In YGNodeCalculateLayout, pass depth as value 0, to indicate the root depth.
  3. Since this depth state is only used during debugging (when gPrintChanges is true), you could further place the increment/decrement in an if-statement that gets optimized out by the compiler when gPrintChanges == false, after further refactoring the latter to be const (or #define'd).

Similarly, we can additionally pass the generation via function calls as above, to avoid the need for a pointer from each node to the tree root, saving another 8 bytes. I suspect the approach used in this PR also breaks cloning, which uses a "copy on write" scheme. In this PR, the pointer to the root is overwritten by YGNodeClone immediately on all children when cloning; but now the cloned nodes are no-longer "shared": we would ideally need the cloned children to point to each of their tree roots; but that becomes awkward as we'd have to manage 1:M pointers, where M is the number of distinct clones. (This doesn't say how we'd then know which of the M pointers to use when performing layout on a cloned subtree; that would be difficult.)

By removing the pointer to the tree root we can also drop the new method setChildRoot which performs a full subtree traversal that can be expensive.

YGNodeRef getRoot();
void setChildRoot(YGNodeRef node);
};
42 changes: 20 additions & 22 deletions yoga/Yoga.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "YGNode.h"
#include "YGNodePrint.h"
#include "Yoga-internal.h"
#include <atomic>
#ifdef _MSC_VER
#include <float.h>

Expand Down Expand Up @@ -222,8 +223,8 @@ void YGNodeMarkDirtyAndPropogateToDescendants(const YGNodeRef node) {
return node->markDirtyAndPropogateDownwards();
}

int32_t gNodeInstanceCount = 0;
int32_t gConfigInstanceCount = 0;
std::atomic<int32_t> gNodeInstanceCount(0);
std::atomic<int32_t> gConfigInstanceCount(0);

WIN_EXPORT YGNodeRef YGNodeNewWithConfig(const YGConfigRef config) {
const YGNodeRef node = new YGNode();
Expand Down Expand Up @@ -254,6 +255,7 @@ YGNodeRef YGNodeClone(YGNodeRef oldNode) {
oldNode->getConfig(),
node != nullptr,
"Could not allocate memory for node");
oldNode->setChildRoot(node);
gNodeInstanceCount++;
node->setOwner(nullptr);
return node;
Expand Down Expand Up @@ -307,8 +309,8 @@ void YGNodeFree(const YGNodeRef node) {

static void YGConfigFreeRecursive(const YGNodeRef root) {
if (root->getConfig() != nullptr) {
gConfigInstanceCount--;
delete root->getConfig();
gConfigInstanceCount--;
}
// Delete configs recursively for childrens
for (auto* child : root->getChildren()) {
Expand Down Expand Up @@ -1068,8 +1070,6 @@ bool YGNodeLayoutGetDidLegacyStretchFlagAffectLayout(const YGNodeRef node) {
return node->getLayout().doesLegacyStretchFlagAffectsLayout;
}

uint32_t gCurrentGenerationCount = 0;

bool YGLayoutNodeInternal(
const YGNodeRef node,
const float availableWidth,
Expand Down Expand Up @@ -1341,8 +1341,7 @@ static void YGNodeComputeFlexBasisForChild(
if (child->getLayout().computedFlexBasis.isUndefined() ||
(YGConfigIsExperimentalFeatureEnabled(
child->getConfig(), YGExperimentalFeatureWebFlexBasis) &&
child->getLayout().computedFlexBasisGeneration !=
gCurrentGenerationCount)) {
child->getLayout().computedFlexBasisGeneration != node->getRoot()->gCurrentGenerationCount)) {
const YGFloatOptional& paddingAndBorder = YGFloatOptional(
YGNodePaddingAndBorderForAxis(child, mainAxis, ownerWidth));
child->setLayoutComputedFlexBasis(
Expand Down Expand Up @@ -1496,7 +1495,7 @@ static void YGNodeComputeFlexBasisForChild(
child->getLayout().measuredDimensions[dim[mainAxis]],
YGNodePaddingAndBorderForAxis(child, mainAxis, ownerWidth))));
}
child->setLayoutComputedFlexBasisGeneration(gCurrentGenerationCount);
child->setLayoutComputedFlexBasisGeneration(node->getRoot()->gCurrentGenerationCount);
}

static void YGNodeAbsoluteLayoutChild(
Expand Down Expand Up @@ -1977,7 +1976,7 @@ static void YGNodeComputeFlexBasisForChildren(
continue;
}
if (child == singleFlexChild) {
child->setLayoutComputedFlexBasisGeneration(gCurrentGenerationCount);
child->setLayoutComputedFlexBasisGeneration(node->getRoot()->gCurrentGenerationCount);
child->setLayoutComputedFlexBasis(YGFloatOptional(0));
} else {
YGNodeComputeFlexBasisForChild(
Expand Down Expand Up @@ -3521,7 +3520,6 @@ static void YGNodelayoutImpl(
}
}

uint32_t gDepth = 0;
bool gPrintTree = false;
bool gPrintChanges = false;
bool gPrintSkips = false;
Expand Down Expand Up @@ -3711,10 +3709,10 @@ bool YGLayoutNodeInternal(
const YGConfigRef config) {
YGLayout* layout = &node->getLayout();

gDepth++;
node->getRoot()->gDepth++;

const bool needToVisitNode =
(node->isDirty() && layout->generationCount != gCurrentGenerationCount) ||
(node->isDirty() && layout->generationCount != node->getRoot()->gCurrentGenerationCount) ||
layout->lastOwnerDirection != ownerDirection;

if (needToVisitNode) {
Expand Down Expand Up @@ -3816,8 +3814,8 @@ bool YGLayoutNodeInternal(
node,
YGLogLevelVerbose,
"%s%d.{[skipped] ",
YGSpacer(gDepth),
gDepth);
YGSpacer(node->getRoot()->gDepth),
node->getRoot()->gDepth);
if (node->getPrintFunc() != nullptr) {
node->getPrintFunc()(node);
}
Expand All @@ -3839,8 +3837,8 @@ bool YGLayoutNodeInternal(
node,
YGLogLevelVerbose,
"%s%d.{%s",
YGSpacer(gDepth),
gDepth,
YGSpacer(node->getRoot()->gDepth),
node->getRoot()->gDepth,
needToVisitNode ? "*" : "");
if (node->getPrintFunc() != nullptr) {
node->getPrintFunc()(node);
Expand Down Expand Up @@ -3873,8 +3871,8 @@ bool YGLayoutNodeInternal(
node,
YGLogLevelVerbose,
"%s%d.}%s",
YGSpacer(gDepth),
gDepth,
YGSpacer(node->getRoot()->gDepth),
node->getRoot()->gDepth,
needToVisitNode ? "*" : "");
if (node->getPrintFunc() != nullptr) {
node->getPrintFunc()(node);
Expand Down Expand Up @@ -3934,8 +3932,8 @@ bool YGLayoutNodeInternal(
node->setDirty(false);
}

gDepth--;
layout->generationCount = gCurrentGenerationCount;
node->getRoot()->gDepth--;
layout->generationCount = node->getRoot()->gCurrentGenerationCount;
return (needToVisitNode || cachedResults == nullptr);
}

Expand Down Expand Up @@ -4039,7 +4037,7 @@ void YGNodeCalculateLayout(
// all dirty nodes at least once. Subsequent visits will be skipped if the
// input
// parameters don't change.
gCurrentGenerationCount++;
node->getRoot()->gCurrentGenerationCount++;
node->resolveDimension();
float width = YGUndefined;
YGMeasureMode widthMeasureMode = YGMeasureModeUndefined;
Expand Down Expand Up @@ -4120,7 +4118,7 @@ void YGNodeCalculateLayout(
originalNode->resolveDimension();
// Recursively mark nodes as dirty
originalNode->markDirtyAndPropogateDownwards();
gCurrentGenerationCount++;
node->getRoot()->gCurrentGenerationCount++;
// Rerun the layout, and calculate the diff
originalNode->setAndPropogateUseLegacyFlag(false);
if (YGLayoutNodeInternal(
Expand Down