Makes Yoga threadsafed:

[nokia6686]

Issue ref: #769

Problem

• gNodeInstanceCount, gConfigInstanceCount, gCurrentGenerationCount, gDepth are global variables in Yoga.cpp. They are not safe when using multithreading.

My idea

• Uses atomic for gNodeInstanceCount and gConfigInstanceCount because of counting instances only.
• Store gCurrentGenerationCount and gDepth in root node for multithreading.

My solution

• Uses for gNodeInstanceCount and gConfigInstanceCount

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

• Every Yoga Node has its root reference (root-ref). New node's root point to itself. Root-ref will be updated when inserting children.
• gCurrentGenerationCount and gDepth are stored in YGNode.
• When updating children, we must update root-ref for each new child.

struct YGNode {
...
  // We must setChildRoot() for new child
  void replaceChild(YGNodeRef oldChild, YGNodeRef newChild);
  void replaceChild(YGNodeRef child, uint32_t index);
  void insertChild(YGNodeRef child, uint32_t index);
...
  private:
    YGNodeRef pRoot = nullptr;

  public:
    uint32_t  gCurrentGenerationCount = 0;
    uint32_t  gDepth = 0;
    YGNodeRef getRoot();
    void setChildRoot(YGNodeRef node);
}
...
void YGNode::setChildRoot(YGNodeRef node) {
  node->pRoot = getRoot();
  for (auto child: node->children_) {
    setChildRoot(child);
  }
}

• Global variables are accessed via root-ref

//gCurrentGenerationCount++;
node->getRoot()->gCurrentGenerationCount++;

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

[nokia6686]

Thank @rockerhieu for support.

[jpap]

I was very recently concerned by Yoga thread safety and saw your PR. Thanks for your contribution.

I see your PR sitting dormant for some time. I'd love to see this topic addressed, so I thought to provide a review with some suggestions for improvement, in the hope we can have an update merged.

If you lack time, or would like some help in refactoring the PR, let me know as I'm happy to help.

[jpap]

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.

[jpap]

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 }

[tcdat96]

@jpap unfortunately nokia6686 left the team several months ago, we are trying to pick up what he left and carry out the pull request with #852.

[NickGerleman]

It looks like this was succeeded by #852 which was merged. Going to close this PR.