Simplify Edge Resolution #42254
Closed
Simplify Edge Resolution #42254
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: Yoga has an odd behavior, where `start`/`end` edges under row-reverse are relative to flex-direction, instead of writing direction. While Yoga doesn't actually document what this behavior is supposed to be, it goes against historic RN documentation, or the behavior valid on the web. It is also applied inconsistently (e.g. sometimes only on container, sometimes on child). It really seems like a bug. We changed the default behavior for Yoga, but left the existing one behind an errata (so existing fbsource users got old behavior). We have previously seen this behavior show up in product code, including CK when running on FlexLayout. `row-reverse` is surprisingly uncommon though: 1. Litho has <40 usages 2. RN has ~40 usages in `RKJSModules`,~30 in `arvr/js` 3. CK has ~80 usages 4. NT has ~40 usages These components are pretty static, and few enough that we can do this style of refactor, where we just fix everything we find. But there is some risk of missing complonents. CK accounts for 17/20 offending usages, since it only exposes start/end edge, and not left/right. It might make sense to make it preserve behavior instead, to reduce risk a bit. FlexLayout is now separately powering Bloks, which wasn't surveryed, do I didn't touch CK behavior under Bloks. There could also be other usages in other frameworks/bespoke usages, and this has implications for OSS users. But based on our own usage, of many, many components, this seems rare. Changelog: [General][Breaking] - Make `start/end` in styles always refer to writing direction Differential Revision: D52698130 fbshipit-source-id: 185661d7d9248f4461044ff93972cdda3b9cd385
Summary: This change aims to simplify how we resolve edges. This operation happens many, many times, and has gotten complex and slow when paired with StyleValuePool. This starts reshaping so that `yoga::Style` can resolve a style prop for a given edge. This is closer to the ideal computed style API to avoid recalcing this so many times, but it is also a good amount cheaper than This relies on removing the errata related to row-reverse, and cleans up the removal started in the last change. This has no measurable perf effect under CompactValue, but has a >10% uplift in perf when using StyleValueHandle, where we can trivially check if a handle points to a defined value without resolving it, but only within `yoga::Style` since we don't expose the handle outside of it. More quantifiably, we go from 2.35 million StyleValuePool reads to 1.05m. Differential Revision: D52605596 fbshipit-source-id: 640327ea5a4334518a1ef9f0183950f638994458
facebook-github-bot
added
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D52605596 |
Base commit: 9bd357e |
facebook-github-bot
pushed a commit
to facebook/yoga
that referenced
this pull request
Jan 19, 2024
Summary: X-link: facebook/react-native#42254 Pull Request resolved: #1550 This change aims to simplify how we resolve edges. This operation happens many, many times, and has gotten complex and slow when paired with StyleValuePool. This starts reshaping so that `yoga::Style` can resolve a style prop for a given edge. This is closer to the ideal computed style API to avoid recalcing this so many times, but doesn't address that. This relies on removing the errata related to row-reverse, and cleans up the removal started in the last change. This has no measurable perf effect under CompactValue, but has a >10% uplift in perf when using StyleValueHandle, where we can trivially check if a handle points to a defined value without resolving it, but only within `yoga::Style` since we don't expose the handle outside of it. More quantifiably, we go from 2.35 million StyleValuePool reads to 993k. The rest are checks on the handle. Reviewed By: joevilches Differential Revision: D52605596 fbshipit-source-id: 0b366963a899e376f99ce3d75cd5f14a25d60cec
facebook-github-bot
pushed a commit
to facebook/litho
that referenced
this pull request
Jan 19, 2024
Summary: X-link: facebook/react-native#42254 X-link: facebook/yoga#1550 This change aims to simplify how we resolve edges. This operation happens many, many times, and has gotten complex and slow when paired with StyleValuePool. This starts reshaping so that `yoga::Style` can resolve a style prop for a given edge. This is closer to the ideal computed style API to avoid recalcing this so many times, but doesn't address that. This relies on removing the errata related to row-reverse, and cleans up the removal started in the last change. This has no measurable perf effect under CompactValue, but has a >10% uplift in perf when using StyleValueHandle, where we can trivially check if a handle points to a defined value without resolving it, but only within `yoga::Style` since we don't expose the handle outside of it. More quantifiably, we go from 2.35 million StyleValuePool reads to 993k. The rest are checks on the handle. Reviewed By: joevilches Differential Revision: D52605596 fbshipit-source-id: 0b366963a899e376f99ce3d75cd5f14a25d60cec
|
This pull request has been merged in 40c4552. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
This change aims to simplify how we resolve edges. This operation happens many, many times, and has gotten complex and slow when paired with StyleValuePool.
This starts reshaping so that
yoga::Stylecan resolve a style prop for a given edge. This is closer to the ideal computed style API to avoid recalcing this so many times, but it is also a good amount cheaper thanThis relies on removing the errata related to row-reverse, and cleans up the removal started in the last change.
This has no measurable perf effect under CompactValue, but has a >10% uplift in perf when using StyleValueHandle, where we can trivially check if a handle points to a defined value without resolving it, but only within
yoga::Stylesince we don't expose the handle outside of it.More quantifiably, we go from 2.35 million StyleValuePool reads to 1.05m.
Differential Revision: D52605596