DaddyFrosty/yoga
1
0
Fork 0
Code Issues 96 Pull Requests 31 Actions Packages Projects Releases 33 Wiki Activity

Massive performance regression since commit fbd332d #734

New Issue
Open
opened 2018-03-15 00:30:11 -07:00 by Edensan · 13 comments
Edensan commented 2018-03-15 00:30:11 -07:00 (Migrated from github.com)
Copy Link

Report

The recent move to "OOP" patterns and code structures has heavily impacted performance as can be seen when running benchmarks:

TAG [1.8.0]
Stack with flex: median: 0.027000 ms, stddev: 0.005840 ms
Align stretch in undefined axis: median: 0.027000 ms, stddev: 0.005836 ms
Nested flex: median: 0.436000 ms, stddev: 0.067741 ms
Huge nested layout: median: 36.938000 ms, stddev: 2.847124 ms

COMMIT [fbd332d]
Stack with flex: median: 0.042000 ms, stddev: 0.007671 ms
Align stretch in undefined axis: median: 0.036000 ms, stddev: 0.009329 ms
Nested flex: median: 0.748000 ms, stddev: 0.108319 ms
Huge nested layout: median: 50.196000 ms, stddev: 3.076368 ms

TAG [1.7.0]
Stack with flex: median: 0.010000 ms, stddev: 0.006567 ms
Align stretch in undefined axis: median: 0.012000 ms, stddev: 0.002526 ms
Nested flex: median: 0.152000 ms, stddev: 0.038512 ms
Huge nested layout: median: 12.710000 ms, stddev: 1.497397 ms

As far as I understand mostly due to unnecessary copies all over the place.
I'm not sure if this is a known issue, but I suggest taking a look at it before it becomes too hard to optimize/refactor.

# Report - [x] I have searched [existing issues](https://github.com/facebook/yoga/issues) and this is not a duplicate The recent move to "OOP" patterns and code structures has heavily impacted performance as can be seen when running benchmarks: TAG [1.8.0] Stack with flex: median: 0.027000 ms, stddev: 0.005840 ms Align stretch in undefined axis: median: 0.027000 ms, stddev: 0.005836 ms Nested flex: median: 0.436000 ms, stddev: 0.067741 ms Huge nested layout: median: 36.938000 ms, stddev: 2.847124 ms COMMIT [fbd332d] Stack with flex: median: 0.042000 ms, stddev: 0.007671 ms Align stretch in undefined axis: median: 0.036000 ms, stddev: 0.009329 ms Nested flex: median: 0.748000 ms, stddev: 0.108319 ms Huge nested layout: median: 50.196000 ms, stddev: 3.076368 ms TAG [1.7.0] Stack with flex: median: 0.010000 ms, stddev: 0.006567 ms Align stretch in undefined axis: median: 0.012000 ms, stddev: 0.002526 ms Nested flex: median: 0.152000 ms, stddev: 0.038512 ms Huge nested layout: median: 12.710000 ms, stddev: 1.497397 ms As far as I understand mostly due to unnecessary copies all over the place. I'm not sure if this is a known issue, but I suggest taking a look at it before it becomes too hard to optimize/refactor.
koxen commented 2018-04-03 00:50:28 -07:00 (Migrated from github.com)
Copy Link

any update?

any update?
emilsjolander commented 2018-04-03 03:08:58 -07:00 (Migrated from github.com)
Copy Link

Thanks for the report! We will look into this!

Thanks for the report! We will look into this!
🎉 1
koxen commented 2018-04-03 07:36:02 -07:00 (Migrated from github.com)
Copy Link

@emilsjolander am I right that this was corrected here cda328fa7e?

@emilsjolander am I right that this was corrected here https://github.com/facebook/yoga/commit/cda328fa7e5392213121edaaebb97b95f60d10f6?
emilsjolander commented 2018-04-03 07:37:58 -07:00 (Migrated from github.com)
Copy Link

@koxen I think so but would be good to confirm

@koxen I think so but would be good to confirm
👍 1
koxen commented 2018-04-03 13:46:04 -07:00 (Migrated from github.com)
Copy Link

@emilsjolander that would be great, would you mind bumping react-native to a beta version after you're done? :)

@emilsjolander that would be great, would you mind bumping react-native to a beta version after you're done? :)
Edensan commented 2018-04-07 19:58:01 -07:00 (Migrated from github.com)
Copy Link

@emilsjolander @koxen
I have been tracking changes and have already profiled this and no, it didn't solve the issue.

Stack with flex: median: 0.033000 ms, stddev: 0.008888 ms
Align stretch in undefined axis: median: 0.032000 ms, stddev: 0.008994 ms
Nested flex: median: 0.561000 ms, stddev: 0.120899 ms
Huge nested layout: median: 39.258000 ms, stddev: 2.278808 ms

@emilsjolander @koxen I have been tracking changes and have already profiled this and no, it didn't solve the issue. Stack with flex: median: 0.033000 ms, stddev: 0.008888 ms Align stretch in undefined axis: median: 0.032000 ms, stddev: 0.008994 ms Nested flex: median: 0.561000 ms, stddev: 0.120899 ms Huge nested layout: median: 39.258000 ms, stddev: 2.278808 ms
Edensan commented 2018-04-07 20:08:15 -07:00 (Migrated from github.com)
Copy Link

There are multiple places in the code where the YGLayout and/or YGStyle are being copied instead of referenced...
eg.

#define YG_NODE_STYLE_PROPERTY_SETTER_IMPL(                               \
    type, name, paramName, instanceName)                                  \
  void YGNodeStyleSet##name(const YGNodeRef node, const type paramName) { \
    if (node->getStyle().instanceName != paramName) {                     \
      YGStyle style = node->getStyle();                                   \
      style.instanceName = paramName;                                     \
      node->setStyle(style);                                              \
      node->markDirtyAndPropogate();                                      \
    }                                                                     \
  }
  1. Style is copied
  2. Value changed
  3. Source style overwritten by copy

............ Why??
Should just be the following:

#define YG_NODE_STYLE_PROPERTY_SETTER_IMPL(                               \
    type, name, paramName, instanceName)                                  \
  void YGNodeStyleSet##name(const YGNodeRef node, const type paramName) { \
    YGStyle& style = node->getStyle();                                    \
    if (style.instanceName != paramName) {                                \
      style.instanceName = paramName;                                     \
      node->markDirtyAndPropogate();                                      \
    }                                                                     \
  }

There are many more such places in the code,

  1. Simple value checks are now becoming method calls that require a little more thought to optimize properly (for instance maybe requires passing YGStyle& and YGLayout& as parameters instead of expecting code to YGNode->getStyle() & YGNode->getLayout() all over the place)
  2. Unnecessary copies of said layouts and styles as seen in example above

I believe this is not a quick fix, this require a more careful review of the current refactoring and potential risks involved.

There are multiple places in the code where the YGLayout and/or YGStyle are being copied instead of referenced... eg. ```cpp #define YG_NODE_STYLE_PROPERTY_SETTER_IMPL( \ type, name, paramName, instanceName) \ void YGNodeStyleSet##name(const YGNodeRef node, const type paramName) { \ if (node->getStyle().instanceName != paramName) { \ YGStyle style = node->getStyle(); \ style.instanceName = paramName; \ node->setStyle(style); \ node->markDirtyAndPropogate(); \ } \ } ``` 1. Style is copied 2. Value changed 3. Source style overwritten by copy ............ Why?? Should just be the following: ```cpp #define YG_NODE_STYLE_PROPERTY_SETTER_IMPL( \ type, name, paramName, instanceName) \ void YGNodeStyleSet##name(const YGNodeRef node, const type paramName) { \ YGStyle& style = node->getStyle(); \ if (style.instanceName != paramName) { \ style.instanceName = paramName; \ node->markDirtyAndPropogate(); \ } \ } ``` There are many more such places in the code, 1. Simple value checks are now becoming method calls that require a little more thought to optimize properly (for instance maybe requires passing YGStyle& and YGLayout& as parameters instead of expecting code to YGNode->getStyle() & YGNode->getLayout() all over the place) 2. Unnecessary copies of said layouts and styles as seen in example above I believe this is not a quick fix, this require a more careful review of the current refactoring and potential risks involved.
Edensan commented 2018-04-07 20:47:25 -07:00 (Migrated from github.com)
Copy Link

Note that I've tried and cleaned up most of the YGStyle and YGLayout copies and still did not change that much, just a few percents which makes me think that there may be something ugly going on somewhere.

Note that I've tried and cleaned up most of the YGStyle and YGLayout copies and still did not change that much, just a few percents which makes me think that there may be something ugly going on somewhere.
woehrl01 commented 2018-04-08 04:24:21 -07:00 (Migrated from github.com)
Copy Link

@Edensan yes, those macros could be adapted, but they aren't heavily used in the benchmark tool.

After having a look throughout the code, my assumption about the performance penality is that the implementation of the YGNode getters aren't decleared directly in the YGNode header. This could lead to the behaviour that these calls aren't inlined correctly (as it only sees the YGNode.h definition and not the concrete implementation inside Yoga.cpp).
I haven't verified that this is the root cause here, but I experienced similar things in other projects.

@Edensan yes, those macros could be adapted, but they aren't heavily used in the benchmark tool. After having a look throughout the code, my assumption about the performance penality is that the implementation of the `YGNode` getters aren't decleared directly in the `YGNode` header. This could lead to the behaviour that these calls aren't inlined correctly (as it only sees the `YGNode.h` definition and not the concrete implementation inside `Yoga.cpp`). I haven't verified that this is the root cause here, but I experienced similar things in other projects.
Edensan commented 2018-04-08 19:08:19 -07:00 (Migrated from github.com)
Copy Link

@woehrl01 Got it.

Since you mentioned it, I suppose refreshing the benchmarks might also be a good idea.

@woehrl01 Got it. Since you mentioned it, I suppose refreshing the benchmarks might also be a good idea.
koxen commented 2018-04-08 20:56:43 -07:00 (Migrated from github.com)
Copy Link

I am seeing noticable performance improvement in my RN app(FlatList, items using lots of flexboxes) after upgrading to version with cda328fa7e
..so it seems related. I would appreciate further pass-by-reference fixes like @Edensan pointed out :)

EDIT: might have been a false positive, looks like changes from master didn't land in 0.55.1 version tag :/

I am seeing noticable performance improvement in my RN app(FlatList, items using lots of flexboxes) after upgrading to version with https://github.com/facebook/yoga/commit/cda328fa7e5392213121edaaebb97b95f60d10f6 ..so it seems related. I would appreciate further pass-by-reference fixes like @Edensan pointed out :) EDIT: might have been a false positive, looks like changes from master didn't land in 0.55.1 version tag :/
Edensan commented 2018-04-16 23:04:50 -07:00 (Migrated from github.com)
Copy Link

Is there someone working on this issue at the moment? Would be nice to keep us updated.

Is there someone working on this issue at the moment? Would be nice to keep us updated.
gengjiawen commented 2019-04-19 04:59:02 -07:00 (Migrated from github.com)
Copy Link

Looks like still not fixed.

From the benchmark, Yoga 1.6.0 is two times faster than 1.14.0.

https://gist.github.com/gengjiawen/141051afa3aa2d6745e86be69c60c2fb

Looks like still not fixed. From the benchmark, Yoga 1.6.0 is two times faster than 1.14.0. https://gist.github.com/gengjiawen/141051afa3aa2d6745e86be69c60c2fb
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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
Algorithm
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#734
No description provided.
Delete Branch "%!s()"

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?