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
nokia6686 commented 2018-07-05 21:25:58 -07:00 (Migrated from github.com)
Copy Link

Issue ref: https://github.com/facebook/yoga/issues/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++;
Issue ref: https://github.com/facebook/yoga/issues/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 <atomic> for `gNodeInstanceCount` and `gConfigInstanceCount` ```C++ 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. ```c++ 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 ```C++ //gCurrentGenerationCount++; node->getRoot()->gCurrentGenerationCount++; //gDepth++; node->getRoot()->gDepth++; ```
👍 1 ❤️ 1 🎉 1
nokia6686 commented 2018-07-05 21:28:03 -07:00 (Migrated from github.com)
Copy Link

Thank @rockerhieu for support.

Thank @rockerhieu for support.
jpap (Migrated from github.com) requested changes 2019-01-04 16:14:14 -08:00
jpap (Migrated from github.com) left a comment
Copy Link

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.

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.
yoga/YGNode.h
@@ -273,0 +275,4 @@
YGNodeRef pRoot = nullptr;
public:
uint32_t gCurrentGenerationCount = 0;
jpap (Migrated from github.com) commented 2019-01-04 15:44:21 -08:00
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 }
yoga/YGNode.h
@@ -273,0 +276,4 @@
public:
uint32_t gCurrentGenerationCount = 0;
uint32_t gDepth = 0;
jpap (Migrated from github.com) commented 2019-01-02 16:57:48 -08:00
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.
tcdat96 commented 2019-01-09 19:34:44 -08:00 (Migrated from github.com)
Copy Link

@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 https://github.com/facebook/yoga/pull/852.

@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 https://github.com/facebook/yoga/pull/852.
👍 1 🎉 1
NickGerleman commented 2022-10-03 09:52:43 -07:00 (Migrated from github.com)
Copy Link

It looks like this was succeeded by https://github.com/facebook/yoga/pull/852 which was merged. Going to close this PR.

It looks like this was succeeded by https://github.com/facebook/yoga/pull/852 which was merged. Going to close this PR.

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
jpap
Labels
Clear labels
Algorithm
Build and Infrastructure
C#
C++ Enhancement
CLA Signed
Discussion
Documentation
Feature Request
Java
JavaScript
Merged
Question
Reverted
Shared with Meta
Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
YogaKit
YogaLayout ViewGroup
bug
dependencies
Pull requests that update a dependency file
duplicate
enhancement
fb-exported
fh:direct-merge-enabled
invalid
ruby
Pull requests that update Ruby code
wontfix
No Label
CLA Signed
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
DaddyFrosty
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#791
No description provided.
Delete Branch "yoga_thread_safety_master"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?