Run commit hooks before layout calculation

[tomekzaw]

Summary

I've noticed that UIManagerCommitHooks are applied after calling layoutIfNeeded, meaning that if a commit hook makes some changes to the tree, it needs to calculate the layout again, effectively making the first calculation redundant.

This PR swaps the order of these two operations and moves shadowTreeWillCommit call before layoutIfNeeded. Thanks to this change, commit hooks don't need to manually trigger layout calculations as well as can potentially operate on unsealed nodes which can make it more efficient in some cases.

Finally, this PR eliminates a crash on emitLayoutEvents(affectedLayoutableNodes); when commit hook actually modifies the tree and thus de-allocates old shadow nodes.

cc @sammy-SC

Changelog

[GENERAL] [CHANGED] - Run commit hooks before layout calculation

Test Plan

The only UIManagerCommitHook I could find in the OSS repo is TimelineController.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,451,936	-17
android	hermes	armeabi-v7a	7,775,337	+4
android	hermes	x86	8,927,580	+18
android	hermes	x86_64	8,785,167	+2
android	jsc	arm64-v8a	6,668,733	-20
android	jsc	armeabi-v7a	7,462,722	+2
android	jsc	x86	9,192,890	+10
android	jsc	x86_64	6,893,883	+1

Base commit: 421df9f
Branch: main

[sammy-SC]

Hello @tomekzaw,

Finally, this PR eliminates a crash on emitLayoutEvents(affectedLayoutableNodes); when commit hook actually modifies the tree and thus de-allocates old shadow nodes.

Do you have a repro for this? This sounds like something we should fix!

Moving the delegate_.shadowTreeWillCommit call to earlier might break the contact. If it is moved to earlier, commit can fail because another thread beats it to it. So we would call shadowTreeWillCommit knowing the commit still can fail. Do you think this can be a problem? Calling shadowTreeWillCommit knowing it may not be comitted sounds

operate on unsealed nodes which can make it more efficient in some cases.

Is this what we want? Having sealed nodes guarantees thread safety and opening up mutation to a hook could lead to hard to reproduce bugs. The API explicitly asks the caller to return a new tree in case something needs to be changed.

Have you ran into any problems with the current implementation of commit hooks? What is the underlaying problem you are trying to solve?

[tomekzaw]

Hello @sammy-SC, thanks for your response!

Do you have a repro for this? This sounds like something we should fix!

Well, kind of. I was able to consistently reproduce this crash in Reanimated's FabricExample app. I'm afraid that preparing a separate minimal reproducible example without react-native-reanimated would take plenty of time since it's not that easy to integrate with Fabric from the perspective of a third-party module. Instead, I can send you the exact commit in Reanimated where the issue occurs as well as include the instructions on how to setup the app so you can investigate the issue (at least on iOS).

Alternatively, I can describe what I've done. Let's assume that my app only changes the width of a <View /> component in a render. I've implemented and registered a UIManagerCommitHook that always makes some change in the tree, for instance changes the height of this view (and thus allocates a new ShadowNode) and returns the new RootShadowNode. When the transaction is committed, affectedLayoutableNodes will contain a raw pointer to the first clone (with changed width) which will unfortunately be deallocated once the hook makes a copy with changed height), and emitLayoutEvents will crash because of bad memory access.

Moving the delegate_.shadowTreeWillCommit call to earlier might break the contact. If it is moved to earlier, commit can fail because another thread beats it to it. So we would call shadowTreeWillCommit knowing the commit still can fail. Do you think this can be a problem? Calling shadowTreeWillCommit knowing it may not be comitted sounds

You're right, the name shadowTreeWillCommit suggests it is invoked immediately before the commit, however in reality the transaction may still fail. On the other hand, in the existing code there's already a call to telemetry.willCommit(); even before the transaction is executed. What if we rename this callback to shadowTreeWillLayout? This is exactly what I'm trying to achieve.

Having sealed nodes guarantees thread safety and opening up mutation to a hook could lead to hard to reproduce bugs. The API explicitly asks the caller to return a new tree in case something needs to be changed.

That's right, technically the current API forces the user to return a new RootShadowNode but obviously it can re-use the existing ShadowNodes. The difference here is that previously the ShadowNodes were already sealed when accessing them in a commit hook (so you would need to clone each ShadowNode along with the path to the root to make any changes) and after this change they would be still unsealed. This would make it possible to modify the original ShadowNode which is still kept in React.js FiberNodes as ShadowNodeWrapper and can be used as a source ShadowNode when calling cloneWithNewProps or other variant.

Also, commit hooks are executed synchronously on the same thread (usually the background thread) so this change shouldn't introduce any new problems in terms of thread safety. Technically, the whole ShadowTree should be sealed even earlier while still on the JS thread (e.g. in completeRoot) because after the jump to the background thread, ShadowNodes can be still accessed from the JS thread, but it doesn't make much sense to do so I think.

One solution might be to seal the nodes first, then let the commit hooks run and finally calculate the layout, but I don't it's currently possible to calculate layout for a sealed ShadowNode. Maybe we should have two separate flags for sealing, one for ShadowNode updates (props, children and state) and one for layout calculation (Yoga node)?

Have you ran into any problems with the current implementation of commit hooks? What is the underlaying problem you are trying to solve?

The current implementation of runs commit hooks after layout calculation, so if my commit hook makes some layout-related changes to the tree, I need to recalculate the layout again. It makes no sense to calculate the layout twice since the first calculation is effectively redundant. Ideally, we should calculate the layout only once on the final ShadowTree after all commit hooks are executed.

While the above is just a minor inconvenience (it's pretty bad in terms of performance though when the commit happens on the UI thread, as it does in our case), the real problem here is the crash on emitLayoutEvents. As a workaround, we could store the oldRootShadowNode in some private field to prevent it from deallocating since we receive it as RootShadowNode::Shared const & but I don't think this should be a responsibility of a commit hook.

TL;DR I understand why it's safer for UIManagerCommitHook to operate on sealed nodes but I don't see the reason to calculate the layout before running commit hooks.

[facebook-github-bot]

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

[tomekzaw]

Hey @sammy-SC, any update on this PR? My idea was to seal the nodes before running commit hooks and then calculate layout, but once the nodes are sealed we can no longer calculate layout without cloning the whole tree. This problem can be solved by having two sealed flags – one for props, children and state inside ShadowNode and one for layout calculation inside LayoutableShadowNode. What do you think?

[sammy-SC]

I started to import this PR but there are some internal CI failures (looks like they are unrelated to this change).

Do you propose implementing two sealed flags in this PR before merging it?

[tomekzaw]

I started to import this PR but there are some internal CI failures (looks like they are unrelated to this change).

Great news, thanks!

Do you propose implementing two sealed flags in this PR before merging it?

The approach with two sealed flags should do the job here but I'm afraid it requires too much changes in the existing code and can be confusing in the future. If possible, let's leave this PR as it is in its current state.

[sammy-SC]

Ok, @tomekzaw sounds good.

[facebook-github-bot]

@sammy-SC merged this pull request in 8d0b5af.