Skip to content

Make Yoga thread safe#786

Closed
nokia6686 wants to merge 3 commits into
react:masterfrom
nokia6686:yoga_thread_safety_master
Closed

Make Yoga thread safe#786
nokia6686 wants to merge 3 commits into
react:masterfrom
nokia6686:yoga_thread_safety_master

Conversation

@nokia6686

Copy link
Copy Markdown

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++;

@facebook-github-bot

Copy link
Copy Markdown
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot

Copy link
Copy Markdown
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

- Uses <atomic> for gNodeInstanceCount & gConfigInstanceCount because of counting instance only.
- Store gCurrentGenerationCount & gDepth in root node for multithreading.
@nokia6686 nokia6686 closed this Jul 6, 2018
@nokia6686 nokia6686 deleted the yoga_thread_safety_master branch July 6, 2018 04:05
jpap added a commit to jpap/yoga that referenced this pull request Jan 17, 2019
jpap added a commit to jpap/yoga that referenced this pull request Jan 17, 2019
See also: discussions in PR react#852 and react#786.

In addition to improving thread safety, it removes 8 bytes of heap per node.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants