Skip to content

Conversation

@nokia6686
Copy link

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
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 [email protected]. Thanks!

@facebook-github-bot
Copy link
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 facebook#852 and facebook#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