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

Add config version, and invalidate layout on config change #1674

Closed
acoates-ms wants to merge 3 commits from configversion into main
pull from: configversion
merge into: DaddyFrosty:main
Conversation 5 Commits 3 Files Changed 7 +213 -5
acoates-ms commented 2024-07-01 11:19:45 -07:00 (Migrated from github.com)
Copy Link

This is a continuation of the previous PR: https://github.com/facebook/react-native/pull/45047

I made the change more generic for allowing any kind of config change to invalidate layout.

This is a continuation of the previous PR: https://github.com/facebook/react-native/pull/45047 I made the change more generic for allowing any kind of config change to invalidate layout.
vercel[bot] commented 2024-07-01 11:19:51 -07:00 (Migrated from github.com)
Copy Link

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
yoga-website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 2, 2024 2:22pm
[vc]: #sJpE9r3Y/JZ0q0rJ6uFNJFyCH8BGWoytPilRbP9+Byo=:eyJpc01vbm9yZXBvIjp0cnVlLCJ0eXBlIjoiZ2l0aHViIiwicHJvamVjdHMiOlt7Im5hbWUiOiJ5b2dhLXdlYnNpdGUiLCJyb290RGlyZWN0b3J5Ijoid2Vic2l0ZSIsImluc3BlY3RvclVybCI6Imh0dHBzOi8vdmVyY2VsLmNvbS9mYm9wZW5zb3VyY2UveW9nYS13ZWJzaXRlLzM2Z3J3MjJDcGhCTlJtVFN0QUpZcVdCSnpNY2MiLCJwcmV2aWV3VXJsIjoieW9nYS13ZWJzaXRlLWdpdC1mb3JrLWFjb2F0ZXMtbXMtY29uZmlndmVyc2lvbi1mYm9wZW5zb3VyY2UudmVyY2VsLmFwcCIsIm5leHRDb21taXRTdGF0dXMiOiJERVBMT1lFRCIsImxpdmVGZWVkYmFjayI6eyJyZXNvbHZlZCI6MCwidW5yZXNvbHZlZCI6MCwidG90YWwiOjAsImxpbmsiOiJ5b2dhLXdlYnNpdGUtZ2l0LWZvcmstYWNvYXRlcy1tcy1jb25maWd2ZXJzaW9uLWZib3BlbnNvdXJjZS52ZXJjZWwuYXBwIn19XX0= **The latest updates on your projects**. Learn more about [Vercel for Git ↗︎](https://vercel.link/github-learn-more) | Name | Status | Preview | Comments | Updated (UTC) | | :--- | :----- | :------ | :------- | :------ | | **yoga-website** | ✅ Ready ([Inspect](https://vercel.com/fbopensource/yoga-website/36grw22CphBNRmTStAJYqWBJzMcc)) | [Visit Preview](https://vercel.live/open-feedback/yoga-website-git-fork-acoates-ms-configversion-fbopensource.vercel.app?via=pr-comment-visit-preview-link&passThrough=1) | 💬 [**Add feedback**](https://vercel.live/open-feedback/yoga-website-git-fork-acoates-ms-configversion-fbopensource.vercel.app?via=pr-comment-feedback-link) | Jul 2, 2024 2:22pm |
NickGerleman (Migrated from github.com) requested changes 2024-07-02 04:27:32 -07:00
NickGerleman (Migrated from github.com) left a comment
Copy Link

This looks good! But I think it introduces subtle bug that breaks layout caching in Fabric, around assigning new config to already created node.

In current Fabric impl, we lazily clone any committed ShadowNodes when we know we may need to write new layout information to them. This currently means new inline Yoga node and Yoga config in the ShadowNode. The new config is associated with cloned node via YGNodeSetConfig, and right now this only dirties the node if meaningful config values have changed.

After this change, if we have new config, derived from previous one, they may be structurally equal, but have a different revision count. So the process of building new config, derived from previous, may still lead to invalidation. Or, in the reverse, a config revision ID is no longer valid if a new config is assigned, and we should clear it (though we do dirty anyways).

I think, we can address this, by adding code around the existing equality check (IIRC in Node::setConfig()), to update kept revision ID to match new config revision ID if they are equal, or clear if not.

This looks good! But I think it introduces subtle bug that breaks layout caching in Fabric, around assigning new config to already created node. In current Fabric impl, we lazily clone any committed ShadowNodes when we know we may need to write new layout information to them. This currently means new inline Yoga node and Yoga config in the ShadowNode. The new config is associated with cloned node via `YGNodeSetConfig`, and right now this only dirties the node if meaningful config values have changed. After this change, if we have new config, derived from previous one, they may be structurally equal, but have a different revision count. So the process of building new config, derived from previous, may still lead to invalidation. Or, in the reverse, a config revision ID is no longer valid if a new config is assigned, and we should clear it (though we do dirty anyways). I think, we can address this, by adding code around the existing equality check (IIRC in `Node::setConfig()`), to update kept revision ID to match new config revision ID if they are equal, or clear if not.
NickGerleman (Migrated from github.com) approved these changes 2024-07-02 11:12:56 -07:00
NickGerleman (Migrated from github.com) left a comment
Copy Link

LGTM

LGTM
facebook-github-bot commented 2024-07-02 11:16:10 -07:00 (Migrated from github.com)
Copy Link

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

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff [on Phabricator](https://www.internalfb.com/diff/D59286992).
facebook-github-bot commented 2024-07-03 12:50:29 -07:00 (Migrated from github.com)
Copy Link

@NickGerleman merged this pull request in facebook/yoga@e4fe14ab3e.

@NickGerleman merged this pull request in facebook/yoga@e4fe14ab3e2ea4a345c757f1c7f0db8a6c284bf2.

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
NickGerleman
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
Merged
Shared with Meta
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#1674
No description provided.
Delete Branch "configversion"

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?