Restore checking shadow tree commit cancellation after commit hook execution #38715
Conversation
|
Hi @michalmaka! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign 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 to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
Base commit: bfd5fe2 |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
facebook-github-bot
added
CLA Signed
| @@ -374,11 +379,6 @@ CommitStatus ShadowTree::tryCommit( | |||
|
|
|||
| auto newRevisionNumber = oldRevision.number + 1; | |||
|
|
|||
| if (!newRootShadowNode || | |||
There was a problem hiding this comment.
I think it's ok to remove the !newRootShadowNode check from here because we already checked it in the new block you added, but I think we should keep the rest. Layout is relatively slow so it could happen that the commit is requested to yield during layout, and we should still be able to cancel then. Changing this behavior without testing its impact would be risky.
There was a problem hiding this comment.
sure thing.
By the way, what do you think about restoring that block with moving it just after getting a lock and before checking for revision numbers' mismatch? It may allow developers to cancel commit instead of getting failure on specific cases and shouldn't cause any behaviour changes
There was a problem hiding this comment.
I've pushed suggested changes, @rubennorte give me a shout about your opinion on that approach
There was a problem hiding this comment.
Yeah, I think that makes sense! thanks!
|
@ryancat has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
…ecution (#38715) Summary: Hello! This PR is a fix for one merged some time ago (#36216). In the PR check for `nullptr` value of `newRootShadowNode` just after performing commit hooks was overlooked. This PR restores previous behaviour of conditional commit cancellation after commit hook execution. ## Changelog: [INTERNAL] [FIXED] - Restore checking shadow tree commit cancellation after commit hook execution Pull Request resolved: #38715 Test Plan: Just register a commit hook that return `nullptr`. In that case current code crashes due to `nullptr` dereference. Reviewed By: sammy-SC Differential Revision: D47972245 Pulled By: ryancat fbshipit-source-id: 7599ad11ed4b2dcaf25e53f676ec4530e37410d5
…ecution (#38715) Summary: Hello! This PR is a fix for one merged some time ago (#36216). In the PR check for `nullptr` value of `newRootShadowNode` just after performing commit hooks was overlooked. This PR restores previous behaviour of conditional commit cancellation after commit hook execution. ## Changelog: [INTERNAL] [FIXED] - Restore checking shadow tree commit cancellation after commit hook execution Pull Request resolved: #38715 Test Plan: Just register a commit hook that return `nullptr`. In that case current code crashes due to `nullptr` dereference. Reviewed By: sammy-SC Differential Revision: D47972245 Pulled By: ryancat fbshipit-source-id: 7599ad11ed4b2dcaf25e53f676ec4530e37410d5
…ecution (facebook#38715) Summary: Hello! This PR is a fix for one merged some time ago (facebook#36216). In the PR check for `nullptr` value of `newRootShadowNode` just after performing commit hooks was overlooked. This PR restores previous behaviour of conditional commit cancellation after commit hook execution. ## Changelog: [INTERNAL] [FIXED] - Restore checking shadow tree commit cancellation after commit hook execution Pull Request resolved: facebook#38715 Test Plan: Just register a commit hook that return `nullptr`. In that case current code crashes due to `nullptr` dereference. Reviewed By: sammy-SC Differential Revision: D47972245 Pulled By: ryancat fbshipit-source-id: 7599ad11ed4b2dcaf25e53f676ec4530e37410d5
## Summary During investigation of race between RN and reanimated shadow tree committing I've noticed that if JS thread commits a new tree (triggering `pleaseSkipReanimatedCommit`) after starting `commit` from reanimated, the latter `commit` may cause JS commit failures. What's more, additional calls to `shouldReanimatedSkipCommit ` result in avoiding CPU consuming copy operations on nodes and thus seems to work more performant. I added those checks just before each operation handling and just after full commit. Chessboard example seemed to work a bit faster that without those changes. I used also profiler and in overall CPU spends less time in ShadowTree. NOTICE⚠️ This change requires change in RN codebase. PR has been submitted facebook/react-native#38715 ## Test plan Check performance of e.g. chessboard example.
Summary:
Hello! This PR is a fix for one merged some time ago (#36216). In the PR check for
nullptrvalue ofnewRootShadowNodejust after performing commit hooks was overlooked. This PR restores previous behaviour of conditional commit cancellation after commit hook execution.Changelog:
[INTERNAL] [FIXED] - Restore checking shadow tree commit cancellation after commit hook execution
Test Plan:
Just register a commit hook that return
nullptr. In that case current code crashes due tonullptrdereference.