make yoga threadsafe #852

tcdat96 · 2019-01-08T10:46:39Z

Continuing #791
@nokia6686 is a former member of our team, so we are trying to pick up what he left and carry out the pull request.

Solution

Improved from previous solution with @jpap's suggestions.

Using atomic for gNodeInstanceCount and gConfigInstanceCount

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

Passing gDepth and gCurrentGenerationCount (renamed to depth and generationCount respectively) between function calls that stem from YGNodeCalculateLayout.
In YGNodeCalculateLayout, pass depth as value 0, to indicate the root depth.
Using #define with gPrintChanges and gPrintSkips to optimized out unnecessary logging and incrementing depth state.

#define PRINT_CHANGES false
#define PRINT_SKIPS false

Prevent memory leak by removing static keyword in YGConfigGetDefault

facebook-github-bot · 2019-01-08T10:46:48Z

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 · 2019-01-08T10:49:18Z

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

tcdat96 · 2019-01-08T11:25:54Z

The check also failed with master branch with the same error (https://travis-ci.org/facebook/yoga/builds/475317585), I think it should be irrelevant to this pull request.

jpap

Thanks for taking the time to refactor the PR, @tcdat96. Some further comments given.

yoga/Yoga.cpp

yoga/YGMacros.h

jpap · 2019-01-15T10:56:34Z

yoga/Yoga.cpp

@@ -240,6 +240,11 @@ YGNodeRef YGNodeClone(YGNodeRef oldNode) {
      "Could not allocate memory for node");
  gNodeInstanceCount++;
  node->setOwner(nullptr);
+
+  YGNodeRef root = node;
+  for (; root->getParent() != nullptr; root = root->getParent());


Looks like this loop always terminates early: the parent is set to nullptr above the loop on line 242.

Might be a good idea to add a test? ;-)

Oh my apologies, thought it just a simple loop, so didn't test it thoroughly :-(
Okay, let me add a test 👍

No worries.

Thinking about this a bit more, I'm not sure we need to do this extra work on clone: the generation is incremented each time we perform a YGNodeCalculateLayout, so even if we preserved the root generation counter for a cloned subtree, we're still incrementing it on a call to YGNodeCalculateLayout for that subtree and before we proceed with any work. Any comparison to the old generation count will be false.

Is the counter used to help avoid node recalculations during multiple passes of a given layout (generation) calculation? I'd need to spend some time to really understand how the generation counter is used for this; and unfortunately the code isn't terribly straightforward or self-documenting. If you're on the FB team, can you ask the original author for a short-cut?

It might be that all is needed is for the generation counter to be monotonically increase on each call to YGNodeCalculateLayout, which means we'll still have to stash it in the root node (if so, the clone stuff should remain). What happens when it wraps to zero?

Unfortunately we aren't from the FB team and neither do we have a full understanding how generetionCount works. My team encounter the race condition problem several months ago, so we opened the previous pull request to try to fix that.
I actually thought you are the one on the FB team ;-)

I would suggest us to stick to the original idea of #791 because it focus on solving the data race condition issue without worrying about optimizing the generationCount itself. This can be done in a separated PR and leave this PR with minimal changes.

I've submitted PR #855 that takes the third approach: it appears to work well!

So from here, let's see if @davidaurelio or others on the FB team can offer their input. @tcdat96, in the meantime if you have time to update your PR with option 2, it can give others an opportunity to review both PRs and weigh in.

Hopefully we can land this thread-safety fix soon. I'm happy to amend my PR if there are preferences on style, etc.

I updated the PR with the first 2 options, but ran into error undefined reference to `pthread_getspecific' when running gtest. Have you ever encountered a similar problem, @jpap?

error undefined reference to `pthread_getspecific' when running gtest.

Sadly I've never experienced that; sounds like a linker configuration problem? Perhaps you've inadvertently modified some of the build configuration someplace. I would try:

Clean and rebuild?

Create a new local branch based off master, then cherry-pick only your changes then rebuild/test.

I've already tried both but unfortunately nothing works. The error appeared even with the master commit so I think it should be my configuration problem, but sadly I don't really have any experience in this type of problem :-(

It seems that the error only emerges in our ubuntu machines, the same source code runs without any issue in MacOS.
I modified the BUCK file in lib/gtest with the workaround described in facebook/buck#1443 and it worked. Now this PR runs successfully with all tests passed.

If you have the time, please take a look at both PRs, @davidaurelio :-)

See also: discussions in PR facebook#852 and facebook#786.

See also: discussions in PR facebook#852 and facebook#786. In addition to improving thread safety, it removes 8 bytes of heap per node.

davidaurelio

Thank you for your contribution. I see good things in here, e.g. passing generation count and depth along as function parameters.

However, I don’t think we really need atomics here. Not using them wouldn’t affect correctness, and avoid the potential bus overhead.
is awesome
please use constexpr – check my inline comments. And please put it in a separate pull request. I will happily merge it.
see inline comment – I don’t agree with the notion of a leak, and would like to retain the behaviour of having a single default config. If you feel strongly about running a destructor when terminating the process, it could be wrapped into a static unique_ptr. But I don’t think we have to do this at all.

davidaurelio · 2019-01-28T19:43:15Z

yoga/YGLayout.h

@@ -22,12 +22,12 @@ struct YGLayout {
  bool doesLegacyStretchFlagAffectsLayout : 1;
  bool hadOverflow : 1;

-  uint32_t computedFlexBasisGeneration = 0;
+  uint8_t computedFlexBasisGeneration = 0;


What was the motivation for this change? Due to alignment requirements, this won’t even save memory without reordering data members.

The motivation is to save heap: there are many bits left over from alignment of the bitfields above it, so you could combine computedFlexBasisGeneration with it. I suspect you can get away with just one bit, so long as the generation counter is the same width (or is truncated with a logical AND mask when the generation is incremented).

yoga/YGMacros.h

yoga/Yoga.cpp

yoga/Yoga.h

yoga/Yoga.cpp

tcdat96 · 2019-03-05T08:25:01Z

@davidaurelio @jpap Sorry for delaying this for too long, I was a little too busy last month.

In the new update, I've:

reverted the atomics
reverted the macros. I'll switch to constexpr in another pull request
reverted the static mistake

So this pull request will only have 2 changes now:

using uint8_t for generationCount
passing depth and generationCount via function parameters, instead of using global variables.

tcdat96 · 2019-03-15T10:13:43Z

any update, @davidaurelio ?

rockerhieu · 2019-04-22T07:46:55Z

@davidaurelio @jpap hello?

appleguy · 2019-05-07T02:35:51Z

Hey all - this is still flagging our thread-sanitizer runs, which makes it impossible to find real issues since it happens so frequency. It looks bad for Yoga's usage at a certain big company :).

Could the team find some way to fix or silence the warning with annotations? cc @davidaurelio

Our team had also previously opened this PR (sat for ~2 months): #770

rockerhieu · 2019-05-29T08:28:48Z

@davidaurelio it's been several months. Could you take a look at this?

davidaurelio · 2019-05-29T16:03:14Z

Hi folks, sorry for being unresponsive.

The aspect of this creating too much noise in sanitizers is new to me, sorry about that. I thought this was mainly due to concerns about the data races themselves.

Let me see how well this merges …

facebook-github-bot

@davidaurelio has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Continuing #791 nokia6686 is a former member of our team, so we are trying to pick up what he left and carry out the pull request. # Solution Improved from previous solution with jpap's suggestions. 2. Passing ```gDepth``` and ```gCurrentGenerationCount``` (renamed to **_depth_** and **_generationCount_** respectively) between function calls that stem from ```YGNodeCalculateLayout```. In ```YGNodeCalculateLayout```, pass ```depth``` as value 0, to indicate the root depth. Pull Request resolved: #852 Reviewed By: SidharthGuglani Differential Revision: D15537450 Pulled By: davidaurelio fbshipit-source-id: 338f51383591ba27702ebe759f6c47c2dede3530

Summary: Continuing facebook/yoga#791 nokia6686 is a former member of our team, so we are trying to pick up what he left and carry out the pull request. # Solution Improved from previous solution with jpap's suggestions. 2. Passing ```gDepth``` and ```gCurrentGenerationCount``` (renamed to **_depth_** and **_generationCount_** respectively) between function calls that stem from ```YGNodeCalculateLayout```. In ```YGNodeCalculateLayout```, pass ```depth``` as value 0, to indicate the root depth. Pull Request resolved: facebook/yoga#852 Reviewed By: SidharthGuglani Differential Revision: D15537450 Pulled By: davidaurelio fbshipit-source-id: 338f51383591ba27702ebe759f6c47c2dede3530

Summary: Continuing facebook#791 nokia6686 is a former member of our team, so we are trying to pick up what he left and carry out the pull request. # Solution Improved from previous solution with jpap's suggestions. 2. Passing ```gDepth``` and ```gCurrentGenerationCount``` (renamed to **_depth_** and **_generationCount_** respectively) between function calls that stem from ```YGNodeCalculateLayout```. In ```YGNodeCalculateLayout```, pass ```depth``` as value 0, to indicate the root depth. Pull Request resolved: facebook#852 Reviewed By: SidharthGuglani Differential Revision: D15537450 Pulled By: davidaurelio fbshipit-source-id: 338f51383591ba27702ebe759f6c47c2dede3530

facebook-github-bot added the CLA Signed label Jan 8, 2019

tcdat96 mentioned this pull request Jan 10, 2019

Makes Yoga threadsafed: #791

Closed

jpap suggested changes Jan 11, 2019

View reviewed changes

yoga/Yoga.cpp Outdated Show resolved Hide resolved

yoga/Yoga.cpp Show resolved Hide resolved

yoga/Yoga.cpp Outdated Show resolved Hide resolved

yoga/Yoga.cpp Outdated Show resolved Hide resolved

jpap reviewed Jan 15, 2019

View reviewed changes

yoga/YGMacros.h Outdated Show resolved Hide resolved

tcdat96 force-pushed the yoga_threadsafe branch 2 times, most recently from 89ce57a to 20d5816 Compare January 15, 2019 10:09

jpap reviewed Jan 15, 2019

View reviewed changes

jpap added a commit to jpap/yoga that referenced this pull request Jan 17, 2019

Addresses thread safety, GitHub issue facebook#769.

761b1d7

See also: discussions in PR facebook#852 and facebook#786.

jpap mentioned this pull request Jan 17, 2019

Addresses thread safety, GitHub issue #769. #855

Closed

jpap added a commit to jpap/yoga that referenced this pull request Jan 17, 2019

Addresses thread safety, GitHub issue facebook#769.

13ebc70

See also: discussions in PR facebook#852 and facebook#786. In addition to improving thread safety, it removes 8 bytes of heap per node.

tcdat96 force-pushed the yoga_threadsafe branch from 20d5816 to 0c96f9f Compare January 17, 2019 09:01

tcdat96 force-pushed the yoga_threadsafe branch 2 times, most recently from 19fa89f to 25caeac Compare January 28, 2019 07:54

davidaurelio suggested changes Jan 28, 2019

View reviewed changes

davidaurelio self-assigned this Feb 1, 2019

switch from global variables to function parameters in Yoga

cdc7512

tcdat96 force-pushed the yoga_threadsafe branch 3 times, most recently from 6f45d65 to cdc7512 Compare March 5, 2019 07:04

facebook-github-bot reviewed May 29, 2019

View reviewed changes

Merge branch 'master' into yoga_threadsafe

a9238c4

facebook-github-bot closed this in facebook/litho@cd0cd89 Jun 25, 2019

facebook-github-bot added the Merged label Jun 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make yoga threadsafe #852

make yoga threadsafe #852

tcdat96 commented Jan 8, 2019

facebook-github-bot commented Jan 8, 2019

facebook-github-bot commented Jan 8, 2019

tcdat96 commented Jan 8, 2019

jpap left a comment

jpap Jan 15, 2019

tcdat96 Jan 15, 2019

jpap Jan 15, 2019 •

edited

Loading

tcdat96 Jan 15, 2019

rockerhieu Jan 16, 2019

jpap Jan 17, 2019 •

edited

Loading

tcdat96 Jan 17, 2019

jpap Jan 18, 2019

tcdat96 Jan 21, 2019

tcdat96 Jan 28, 2019 •

edited

Loading

davidaurelio left a comment

davidaurelio Jan 28, 2019

jpap Jan 30, 2019

tcdat96 commented Mar 5, 2019 •

edited

Loading

tcdat96 commented Mar 15, 2019

rockerhieu commented Apr 22, 2019

appleguy commented May 7, 2019 •

edited

Loading

rockerhieu commented May 29, 2019

davidaurelio commented May 29, 2019

facebook-github-bot left a comment

make yoga threadsafe #852

make yoga threadsafe #852

Conversation

tcdat96 commented Jan 8, 2019

Solution

facebook-github-bot commented Jan 8, 2019

facebook-github-bot commented Jan 8, 2019

tcdat96 commented Jan 8, 2019

jpap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpap Jan 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpap Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcdat96 Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

davidaurelio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcdat96 commented Mar 5, 2019 • edited Loading

tcdat96 commented Mar 15, 2019

rockerhieu commented Apr 22, 2019

appleguy commented May 7, 2019 • edited Loading

rockerhieu commented May 29, 2019

davidaurelio commented May 29, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

jpap Jan 15, 2019 •

edited

Loading

jpap Jan 17, 2019 •

edited

Loading

tcdat96 Jan 28, 2019 •

edited

Loading

tcdat96 commented Mar 5, 2019 •

edited

Loading

appleguy commented May 7, 2019 •

edited

Loading