Summary:
Changelog: [internal]
Background executor performs unnecessary operations when second `completeRoot` message from React arrives before first `completeRoot` was finished. This produces unnecessary `ShadowViewMutations`.
Mechanism:
Everytime `completeRoot` is received, before the call is dispatched on the background queue, `completeRootEventCounter_DO_NOT_USE_` is incremented.
Inside `ShadowTree::tryCommit` we check if the value has been incremented to determine if another `completeRoot` is queued.
Reviewed By: JoshuaGross
Differential Revision: D24419160
fbshipit-source-id: 11e19026feca01db6c8981b093a691a6b58a006f
Summary:
Changelog: [internal]
There are two exceptions inside `TelemetryController::pullTransaction`:
- Empty Optional cannot be unwrapped
- mutex lock failed: Invalid argument
By marking this method `noexcept`, stack trace is lost and it makes it more difficult to track down the issue.
What does compiler do if a method is marked `noexcept`?
```
void f() noexcept {
try {
// do work
}
catch (...) {
std::terminate(); // This is the std::terminate() we are seeing in stack traces.
}
}
```
Removing noexcept specifier might give us more information about the exception.
Reviewed By: JoshuaGross
Differential Revision: D24477861
fbshipit-source-id: 80f26e9ab160a5330c2848b89a01d60bfc0a4611
Summary:
changelog: [internal]
Prevents 2 type converions:
1. int <-> size_t
2. int <-> int32_t
# Why is using size_t better when working with indexes.
## 1. Type conversion isn't for free.
Take this example
```
size_t calculate(int number) {
return number + 1;
}
```
It generates following assembly (generated with armv8-a clang 10.0.0):
```
calculate(int): // calculate(int)
sub sp, sp, #16 // =16
str w0, [sp, #12]
ldr w8, [sp, #12]
add w9, w8, #1 // =1
mov w8, w9
sxtw x0, w8
add sp, sp, #16 // =16
ret
```
That's 9 instructions.
If we get rid of type conversion:
```
size_t calculate(size_t number) {
return number + 1;
}
```
Assembly (generated with armv8-a clang 10.0.0):
```
calculate(unsigned long): // calculate(unsigned long)
sub sp, sp, #16 // =16
str x0, [sp, #8]
ldr x8, [sp, #8]
add x0, x8, #1 // =1
add sp, sp, #16 // =16
ret
```
Compiler now produces only 7 instructions.
## Semantics
When using int for indexing, the type doesn't say much. By using `size_t`, just by looking at the type, it gives the reader more information about where it is coming from.
Reviewed By: JoshuaGross
Differential Revision: D24332248
fbshipit-source-id: 87ef982829ec14906ed9e002ea2e875fda4a0cd8
Summary:
Index adjustment is tricky. Seems more reliable to adjust each immediate mutation, and then immediately adjust delayed mutations based on it, rinse and repeat.
Previously it was possible to construct examples where the UI would get into a weird state because index adjustment caused items to be inserted in the wrong location.
Changelog: [Internal]
Reviewed By: kacieb
Differential Revision: D24232926
fbshipit-source-id: f8c445213528c2d2aedacf3e0c73c5bbeb62bc3d
Summary:
In the new Flattening differ, I experimentally verified that these two code paths are not hit (or redundant) and deleted them.
One of the branches did nothing and the other produced duplicate DELETE mutations for the same tag, that is handled elsewhere.
Changelog: [Internal]
Reviewed By: fkgozali
Differential Revision: D23806161
fbshipit-source-id: 9ad2929e2d719a7b9b34640ed35f7a696103604b
Summary:
See additional assertion. Tests still pass, so no other change was necessary.
Changelog: [Internal]
Differential Revision: D23775553
fbshipit-source-id: 57d3191f25dd55ab4da189207f6d201a31b175e0
Summary:
Currently, MountingCoordinator's RN_SHADOW_TREE_INTROSPECTION code will crash often because it assumes there is always a "new" tree to compare the old tree to. In the LayoutAnimations context this is not always the case - in fact, the majority of the time, LayoutAnimations is producing mutations for animation without a "new" tree.
Just check that the tree exists before trying to print it.
Changelog: [Internal]
Reviewed By: shergin
Differential Revision: D23747289
fbshipit-source-id: a1ba22aeae32ed8915a53bc33cdc199e8ce5128a
Summary:
In MountingCoordinator override mode (used in LayoutAnimations) we must set the start and end `diff` time when no real diff happens, otherwise we will hit an assert in telemetry later.
I also ensure that the TransactionNumber is incremented in that case.
Changelog: [Internal]
Reviewed By: shergin
Differential Revision: D23746684
fbshipit-source-id: b1fe3864e453fdba89d43cc827bd37434abf7a4d
Summary:
We don't need a shared_ptr here and without it the code will be faster and simpler.
This change is aligned with any clone-line callbacks we have in the Core which accepts a `const &` and return `shared_ptr<>`.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: JoshuaGross, sammy-SC
Differential Revision: D23725687
fbshipit-source-id: 1cd959f4273913175d342302e2f12752f0114768
Summary:
Previously, to get a current root shadow node for a shadow tree, we called `tryCommit` method and stole a pointer from this. That was not a very straightforward method to get things done, and most importantly we need to do this to change the shape of the ShadowTreeCommitTransaction signature (remove a shared pointer from the callback) to make it simpler, faster and allow future improvements (see the next diff).
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: JoshuaGross, sammy-SC
Differential Revision: D23725689
fbshipit-source-id: 51950b843a0e401828b6c6a38e5e2aaaf21ec166
Summary:
Instead of storing tree separate instance variables, we now store a single object that contains them. We will need it to be able to postpone the mounting stage (we save all info we need for mounting inside the new instance variable).
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: sammy-SC
Differential Revision: D23725690
fbshipit-source-id: 09dd4a0912c6f5b34d805636b62f73ca12287329
Summary:
The implementation of this class is too complex for the purpose it serves. Making it simpler will make the code simpler and faster.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: sammy-SC
Differential Revision: D23725688
fbshipit-source-id: 5e1ecddb0dd3c4c4f94786e2ba0af9b67e7426ce
Summary:
It's more correct and we will rely on this in the future.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: JoshuaGross
Differential Revision: D23681508
fbshipit-source-id: 5fef0ea164e8a400e6ca9a67947252b47ce6d44e
Summary:
One of the operations we do in `Scheduler::stopSurface()` is committing an empty tree to free up `ShadowNode` objects and "disable" `EventEmitter`s associated with them. Before this change, we had a gap in time between a moment when we commit an empty tree and remove the tree from the registry. During this time gap, JavaScript (or native, actually) can commit another tree and mount another new state on the screen. To prevent this, we remove the tree from the registry first and only then commit an empty tree to a uniquely owned tree.
Note that the deleted comment says that we actually have to have a tree in the registry for committing an empty tree, I don't think it's true now.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: sammy-SC
Differential Revision: D23667882
fbshipit-source-id: 387052e9f3e78e7d4446f36baed50f9caa831133
Summary:
We need it to stop repeating to commit new shadow tree in `ShadowTree::commit` when a transaction cancels the commit.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: JoshuaGross, sammy-SC
Differential Revision: D23603959
fbshipit-source-id: d279fb3bf4190e860740a6450595d6f2fc3117f7
Summary:
In `StateReconciliationTest`, the way initializer lists are used to create null `ShadowNode`s causes this error on Visual Studio 2017 on Windows:
```cpp
auto result = (ShadowNode const *){nullptr};
```
---
```
StateReconciliationTest.cpp(35): error C4576: a parenthesized type followed by an init
ializer list is a non-standard explicit type conversion syntax
```
This change allows this test to compile in Visual Studio 2017, and the effected tests successfully compile and pass on Windows. They also compile and pass on Linux and macOS (both built with Clang)
## Changelog
Changelog: [Internal][Changed] - Fabric Tests: Change null ShadowNode creation in StateReconciliationTest
Pull Request resolved: https://github.com/facebook/react-native/pull/29899
Test Plan: The Fabric test suite passes on Windows after this change is made. I also tested it under macOS and Linux built with Clang and they both pass with this change made.
Reviewed By: sammy-SC
Differential Revision: D23592007
Pulled By: shergin
fbshipit-source-id: 7c6131736d478a0bf29d6c9475ef9149b7602dd6
Summary:
Shadow tree introspection was disabled for a while, now we need it back working. This diff also restructures the logic of `MountingCoordinator::pullTransaction()` splitting it into two sections, first one for the base case and the second for the overriding case.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: mdvacca
Differential Revision: D23374948
fbshipit-source-id: 0b5f1c598975bceb3dcb6a0eaee67ff58ef9dda1
Summary:
Just renaming, nothing more.
The idea of MountingTelemetry already grown to something bigger than just mounting telemetry, so we are renaming it.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: mdvacca
Differential Revision: D23374947
fbshipit-source-id: f60ce38b75d1ce77498b84688e59598314c69a78
Summary:
Now we store a revision number of a Shadow Tree that leads to a transaction for which the concrete instance of MountingTelemetry corresponds. This is useful to understand how many actual transactions were skipped during a mounting phase (a mounting transaction does not directly correspond to a commit operation).
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: mdvacca
Differential Revision: D23364663
fbshipit-source-id: 32b86bcdfc1ae97d8fff3b97a8615cc5a5b4d4a9
Summary:
With this change, we now collect the number of text measurements that we perform during the layout phase of the commit. Text measurements are the most expensive layout operations which pretty much responsible for the vast majority of time spent in the layout phase.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: mdvacca
Differential Revision: D23364664
fbshipit-source-id: 19514b93166b4053c2f3be37e79507f2c5248000
Summary:
# What is this?
For a very long time, we've discussed the possibility of detecting Node Reparenting in the Fabric Differ. Practically, from the developer perspective, ReactJS and React Native do not allow reparenting: nodes cannot be reparented, only deleted and then recreated with entirely new tags.
However, Fabric introduced the idea of View Flattening where views deemed unnecessary would be removed from the View hierarchy entirely. This is great and improves memory usage, except for one issue: if a View becomes unflattened, or becomes flattened, the entire tree underneath it must be rebuilt.
In a past diff we introduced a mechanism to detect sibling reordering cleverly, and produce a minimal instruction set. This diff is very similar: we know the invariants around flattening and unflattening of views and we take advantage of them to produce an optimal set of instructions efficiently.
# What's different from previous attempts?
No global maps! Those are slow!
This seems to work and (hopefully) might even improve performance, since way less work is being done on the UI thread in cases when views are (un)flattened.
This *only* does extra work when flattening/unflattening happens, which gives product engineers a little more control over perf.
# So, how's it work?
This algorithm is intuitively simple (I think) but tricky to pull off, because there are lots of edge-cases.
In short: In the past, that information was hidden from the Differ: the differ didn't know if views were being reparented, it would see them
as entirely new views or as views being deleted if a View was flattened or unflattened. We very subtly change the information given to the differ:
all nodes are visible to the differ, but marked as Flattened or Unflattened. Thus, when the differ compares two nodes in the "old" and "new" tree,
it can tell not just if there are updates to the node but if it has been unflattened or flattened as well.
For example, take this tree, where * indicates that a View is flattened:
```
A
+
+----+---+
B* X
+ +
| |
+---+--+ +
E F Y
```
When the Differ asks for the children of A, in the past it would get a list `[E, F, X]`. That is, B* and X are both its children, but since B is flattened, it is omitted entirely from the list and
its children are substituted.
Now, when the Differ asks for the children of A, we give it this list instead: `[B*, E, F, X]`. That is: we give it a list which includes B, but B is marked as flattened.
Another wrinkle: A node `X` could have its children flattened, but still be a concrete view: so flattening/unflattening is a different operation from making a view "concrete" or "unconcrete", which can change independently of flattening.
There is one additional wrinkle: because of zIndex/stacking order, the children of `B` might not actually appear after `B` in the list. Depending on zIndex, a tree that looks like this:
```
A
+
+------+------+
B* C*
+ +
| |
+--+--+ +--+--+
D E F G
```
Could actually be linearized as: `[D G B* F C* E]` (as an extreme example; but basically all permutations as possible).
This is the reason, and the *only* reason that the inner Flattener/Unflattener
## The cases we need to handle
There are 7 cases/edge-cases of flattening and unflattening that we need to handle. Practically, all cases of reordering + flattening/unflattening, and taking recursive cases into account:
1. View A and A' (A in the old tree, A' in the new tree) are matched in the differ, and A* has been flattened or unflattened. These two cases are the easiest to handle.
2. View A' has been reordered with its siblings, and has been flattened or unflattened. These cases are slightly trickier to handle.
3. While flattening or unflattening, we encounter a child that has also been unflattened or flattened. So we need to handle four cases here in total: Flatten-Flatten, Flatten-Unflatten, Unflatten-Flatten, and Unflatten-Unflatten.
Other things to think about, also covered above:
1. Ordering. Views can be reordered and flattened/unflattened at the same time.
2. zIndex ordering: children in a certain order from the ShadowNode perspective may be stacked differently from a View perspective. We use the zIndex ordering for everything in the differ, and this prevents us from performing certain optimizations (see above: we cannot assume that children come after their parent in a list; they may come before, may be interwoven with children from other parents, etc).
# Perf Implications?
Practically, there should be very little negative overhead. There is some overhead in actually performing a flattening/unflattening operation, but... not much more than before. We don't use global maps, so the cost of flattening/unflattening is basically `O(number of nodes reparented)` - note that that's direct nodes reparented, *not* descendants.
tl;dr the perf hit should be similar to reordering, which is non-zero, but close to zero, and zero-cost for any diff operations on parts of the tree that don't involve flattening/unflattening. AFAICT this is very close to an ideal solution for that reason (but I wish it was simpler overall).
# In Summary?
I hope this works out and I think it could improve a number of things downstream: perf, LayoutAnimations, Bindings, certain crashes because of platform assumptions about mutations, etc.
Is it worth it? This new implementation is substantially harder to reason about, harder to read, and harder to understand. This is an important consideration. All I can say there is that I trust the test suite I've been using, but
the decreased readability is a big negative. Hopefully we can improve this in the future.
The rest is fiddly implementation details that I sincerely hope can be improved and simplified in the future.
# Followups?
The part that makes this algorithm the most expensive is that because of zIndex ordering, we cannot assume that children are linearized after their parents and so we rely more heavily on maps for the flattening/unflattening. Our TinyMap implementation should make these `find` operations fast enough unless trees' children are constantly being reordered, but it's still worth thinking of ways to make this even faster.
Changelog: [Internal]
Reviewed By: shergin, mdvacca
Differential Revision: D23259341
fbshipit-source-id: 35d9b90caf262d601a31996ea2cb37e329c61ffc
Summary:
1. When testing major changes to the differ, it can be useful to have more verbose logging.
2. On Android, since asserts don't fire yet, I log which asserts are failing.
Should have no impact on any builds unless you manually set the macro here, and it will have no impact on production builds regardless.
Changelog: [Internal]
Reviewed By: shergin
Differential Revision: D23257859
fbshipit-source-id: 94a8e74ece8023064de0f2203db6074975f8f1f0
Summary:
Partial backout of D23123575 (https://github.com/facebook/react-native/commit/1e4d8d902daca8e524ba67fc3c1f4b77698c4d08). It's causing some crashes and there is a more efficient way of doing it, which I will land in a future diff.
Leaving unused feature-flags in place for now, they'll be used shortly.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D23198625
fbshipit-source-id: 6e9cbc6b39898a604b8f4dfccf5a6dd238511a68
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 creates the Android OSS build system for the module react/utils
As part of this diff I also moved the module to react/utils folder
changelog: [internal] internal
Reviewed By: JoshuaGross
Differential Revision: D22877265
fbshipit-source-id: 717487aacb392d0f08530763a16a638b8021d501
Summary:
Now SurfaceTelemetry records the last 16 full transaction telemetries. We will use it info to report to our trackers.
Changelog: [Internal] Fabric-specific internal change.
Reviewed By: mdvacca
Differential Revision: D22887633
fbshipit-source-id: 0d88adff757e4bc5a701b51d4d06d85e1f51f10f
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
Joshua Gross
generatedunixname89002005287564
Valentin Shergin
empyrical
generatedunixname89002005287564@sandcastle995.atn2.facebook.com
Stephy Ma
David Vacca