DaddyFrosty/yoga
1
0
Fork 0
Code Issues 96 Pull Requests 31 Actions Packages Projects Releases 33 Wiki Activity

Makes Yoga threadsafed: #791

Closed
nokia6686 wants to merge 2 commits from yoga_thread_safety_master into main
pull from: yoga_thread_safety_master
merge into: DaddyFrosty:main
Conversation 4 Commits 2 Files Changed 3 +44 -22
3 changed files with 44 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
yoga/YGNode.cpp
View File

@@ -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) {
@@ -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
yoga/YGNode.h
View File

@@ -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;
jpap commented 2019-01-04 15:44:21 -08:00 (Migrated from github.com)
Review
Copy Link

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 }
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;
jpap commented 2019-01-02 16:57:48 -08:00 (Migrated from github.com)
Review
Copy Link

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.

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
yoga/Yoga.cpp
View File

@@ -14,6 +14,7 @@
#include "YGNode.h"
#include "YGNodePrint.h"
#include "Yoga-internal.h"
#include <atomic>
#ifdef _MSC_VER
#include <float.h>
@@ -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();
@@ -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;
@@ -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()) {
@@ -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,
@@ -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(
@@ -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(
@@ -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(
@@ -3521,7 +3520,6 @@ static void YGNodelayoutImpl(
}
}
uint32_t gDepth = 0;
bool gPrintTree = false;
bool gPrintChanges = false;
bool gPrintSkips = false;
@@ -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) {
@@ -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);
}
@@ -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);
@@ -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);
@@ -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);
}
@@ -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;
@@ -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(