Summary:
changelog: [internal]
Add more clang tidy rules to prevent common class of bugs.
Reviewed By: javache
Differential Revision: D39245194
fbshipit-source-id: 5521c5c4653d7005b96ebba494e810ba5075afbc
Summary:
Fixes https://github.com/facebook/react-native/issues/34120
The new React Native architecture doesn't check `needsCustomLayoutForChildren` so it wrongly positions native views on Android. In https://github.com/facebook/react-native/issues/34120 there are videos comparing the positioning of a native action view in the old and the new architecture.
This PR passes the parent tag to the `updateLayout` method of the `SurfaceMountingManager`. The `SurfaceMountingManager` calls `needsCustomLayoutForChildren` on the parent view manager (copied the code from the `NativeViewHierarchyManager` in the old architecture).
**NOTE** - I wasn't sure where to get the parent shadow view from so I've put in my best guesses where I could and left it as `{}` otherwise.
## Changelog
[Android] [Fixed] - Migrate `needsCustomLayoutForChildren` check to the new architecture
Pull Request resolved: https://github.com/facebook/react-native/pull/34254
Test Plan:
I checked the fix in the repro from https://github.com/facebook/react-native/issues/34165. Here is a video of the action view closing using the native button that is now visible in the new architecture.
https://user-images.githubusercontent.com/1761227/180607896-35bf477f-4552-4b8a-8e09-9e8c49122c0c.mov
Reviewed By: cipolleschi
Differential Revision: D38153924
Pulled By: javache
fbshipit-source-id: e2c77fa70d725a33ce73fe4a615f6d884312580c
Summary:
TL;DR: For applications using JS navigation, save 50-95% of CPU during mounting phase in N>2 navigations that replace ~most of screen.
During investigation of performance on the UI thread of React Native applications, I noticed that the /initial/ render of an screen for an application using JS navigation is /mostly/ consumed (on the UI thread) by tearing-down the previous View hierarchy. In one 185ms segment on the UI thread in production, 95% of the CPU time was Remove/Delete instructions and only 5% of CPU time was consumed by actually displaying the new hierarchy (this is specific to Android and also assumes that View Preallocation is being used, so post-commit work consists of Insert and UpdateLayout mutations primarily).
There are /some/ cases where the C++ differ knows that we are deleting an entire subtree and therefore we could communicate this to the mounting layer. All that matters is that these Views are removed from the View hierarchy immediately; and secondarily that their memory is cleaned up ASAP, but that doesn't need to happen immediately.
Some additional constraints and notes:
1) As noted in the comments, we cannot simply stop producing Remove and Delete instructions. We need to produce /both/ the new RemoveDeleteTree instruction, /and/ produce all the Remove/Delete instructions, primarily because LayoutAnimations relies heavily on these Remove/Delete instructions and certain things would break if we removed those instructions entirely. However, we can mark those Remove/Delete instructions as redundant, process them only in LayoutAnimations, and not send them to the Android mounting layer.
2) We want to make sure that View Recycling is not impacted. Since Android cannot take advantage of View Recycling until /after/ the second major render (preallocation of views will happen before any views are recycled), this doesn't impact View Recycling and we'll make sure Views are recycled whenever they are deleted.
Thus, we do two things:
1) Introduce a new RemoveDeleteTree operation that can delete an entire subtree recursively as part of one operation. This allows us to avoid serializing hundreds or thousands of instructions and prevents JNI traffic.
2) Besides removing the topmost View from the View hierarchy, and ensuring it's not drawn, the full teardown and recycling of the tree can happen /after/ the paint.
In some flows with JS navigation this saves us 95% of CPU during the mount phase. In the general case it is probably closer to 25-50% of CPU time that is saved and/or deferred.
Changelog: [Android][Changed] Significant perf optimization to Fabric Remove/Delete operations
Reviewed By: ryancat
Differential Revision: D37257864
fbshipit-source-id: a7d33fc74683939965cfb98be4db7890644110b2
Summary:
changelog: [internal]
In this diff, we delete default initialised for ShadowViewMutation to prevent accidentally creating empty ShadowViewMutation.
The other initialiser is made private and all of its uses are migrated to designated initialisers. This makes for safer API.
Reviewed By: RSNara
Differential Revision: D30774900
fbshipit-source-id: d2064bf08409850e75e13ad06558b7980a7f5d8d
Summary:
On Android we have the notion of "virtual views", which are defined consistently but the logic is scattered and duplicated throughout the codebase.
The logic exists to mark nodes that exist in the ShadowTree, but not the View tree. We want to CREATE, UPDATE, and DELETE them on the platform, but not INSERT or REMOVE
them. They basically exist as EventEmitter objects.
The only issue with this is (1) duplicated code, which opened the possibility for inconsistent definition (2) StubViewTree did not account for virtualized views, which caused
assert crashes in debug mode for certain LayoutAnimations on Android.
By moving the definition to ShadowViewMutation and accounting for it in StubViewTree, asserts are correct and consistent on all platforms.
This was not caught until recently, because, until recently, no asserts actually ran on Android.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27001199
fbshipit-source-id: eb29085317037ba8a286d7813bdd57095ad4746f
Summary:
Original commit changeset: 3ed8e78e31b0
Backing-out D25938851 (https://github.com/facebook/react-native/commit/69b3016171bb2f994dd4a62c34c2c4645b5a7d56) and D25935785 (https://github.com/facebook/react-native/commit/bdea479a1faa0f1f7d7c9d9162212cce94bc9720). Based on analysis documented in T83141606, I believe this issue should be fixed in JS.
Additionally, this crash actually has nothing to do with (un)flattening or the differ; it is a side-effect of stale ShadowNodes being cloned, which I believe is either UB or a contract violation. Either way, it should probably be fixed either in JS, or in node cloning. So this isn't the right solution for this issue and should be reverted.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D25949569
fbshipit-source-id: 8cf1094a767da98fff4430da60d223412e029545
Summary:
Android has some optimizations around view allocation and pre-allocation that, in the case of View Unflattening, can cause "Create" mutations to be skipped.
To make sure that doesn't happen, we add a flag to ShadowViewMutation (in the core) that any platform can consume, that indicates if the mutation is a "recreation" mutation.
It is still a bit unclear why this is needed, in the sense that I would expect props revision to increment if a view is unflattened. However, there is at least one documented reproduction where that is *not* the case. So for now, we'll have a hack pending further investigation.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D25935785
fbshipit-source-id: 6fb4f0a6dedba0fe46ba3cd558ac1daa70f671f5
Summary:
The `index` parameter for UpdateMutation is optional, and is normally just -1. It's not useful, so remove it. `parentShadowView` is also not relevant and is not used; in some existing use-cases the actual parent view of the updated view is available, and in some contexts the parent view is not set.
The function now will always set the index to -1 for UpdateMutations, and `{}` for ParentShadowView.
This should have no impact on iOS or Android, as this parameter is not used. It could theoretically have an impact on lifetimes of objects retained (now not retained) by not passing parentShadowView into the mutation. For example, any shared props or state associated with the parent will not be retained in the Update mutation now.
Changelog: [Internal]
Reviewed By: shergin
Differential Revision: D25342943
fbshipit-source-id: 0ddbef76a6e2eefc2629c9729f721d8674d7737e
Summary:
# Summary
In previous diffs earlier in 2020, we made changes to detect and optimize reordering of views when the order of views changed underneath the same parent.
However, until now we have ignored reparenting and there's evidence of issues because of that. Because Fabric flattens views more aggressively, reparenting is also marginally more likely to happen.
This diff introduces a very general Reparenting detection. It will work with view flattening/unflattening, as well as tree grafting - subtrees moved to entirely different parts of the tree, not just a single
parent disappearing or reappearing because of flattening/unflattening.
There is also another consideration: previously, we were generating strictly too many Create+Delete operations that were redundant and could cause consistency issues, crashes, or bugs on platforms that do not handle that gracefully -
especially since the ordering of the Create+Delete is not guaranteed (a reparented view could be created "first" and then the differ could later issue a "delete" for the same view).
Intuition behind how it works: we know the cases where we can detect reparenting: it's when nodes are *not* matched up with another node from the other tree, and we're either trying to delete an entire subtree, or create an entire subtree. For perf reasons, we generate whatever set of operations comes first (say, we generate all the Delete and Remove instructions) and take note in the `ReparentingMetadata` data-structure that Delete and/or Remove have been performed for each tag (if ordering is different, we do the same for Create+Insert if those come first). Then if we later detect a corresponding subtree creation/deletion, we don't generate those mutations and we mark the previous mutations for deletion. This incurs some map lookup cost, but this is only wasteful for commits where a large tree is deleted and a large tree is created, without reparenting.
We may be able to improve perf further for certain edge-cases in the future.
# Why can't we solve this in JS?
Two things:
1. We certainly can avoid reparenting situations in JS, but it's trickier than before because of Fabric's view flattening logic - product engineers would have to think much harder about how to prevent reparenting in the general case.
2. In the case of specific views like BottomSheet that may crash if they're reparented, the solution is to make sure that the BottomSheet and the first child of the BottomSheet is never memoized, so that lifecycle functions and render are called more often; and that in every render, the BottomSheet manually clones its child, so that when the Views are recreated, the child of the BottomSheet has a tag and is an entirely different instance. This is certainly possible to do but feels like an onerous requirement for product teams, and it could be challenging to track down every specific BottomSheet that is memoized and/or hoist them higher in the view hierarchy so they're not reparented as often.
Reviewed By: shergin
Differential Revision: D23123575
fbshipit-source-id: 2fa7e1f026f87b6f0c60cad469a3ba85cdc234de
Summary:
This diff moves fabric C++ code from ReactCommon/fabric to ReactCommon/react/renderer
As part of this diff I also refactored components, codegen and callsites on CatalystApp, FB4A and venice
Script: P137350694
changelog: [internal] internal refactor
Reviewed By: fkgozali
Differential Revision: D22852139
fbshipit-source-id: f85310ba858b6afd81abfd9cbe6d70b28eca7415
Samuel Susla
Graham Mendick
Joshua Gross
Andres Suarez
David Vacca