Summary:
Virtual views that are flattened and don't "FormsView" on-screen should not be preallocated.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D28811419
fbshipit-source-id: 949dcbf4cf3791355c58af785603b35fa50f3f02
Summary:
I am deduping a duplicated block, and adding comments to explain when we create INSERT/REMOVE mutations immediately and when we defer creation.
Theoretically the ordering of mutations will be more consistent now, which ~shouldn't matter, but is probably a decent property to have. In particular, before, in some cases
both of these orderings were possible in various scenarios:
```
INSERT X -> Y
INSERT Y -> Z
```
and
```
INSERT Y -> Z
INSERT X -> Y
```
Both of those are fine/correct/won't cause issues on any known platforms. But now, at least for the two cases touched here, only this ordering will be produced:
```
INSERT Y -> Z
INSERT X -> Y
```
meaning we build the tree from the bottom-up (the "bottom" being the root) and do out-of-order inserts less frequently.
Again, the biggest part of this diff should be readability/refactoring/de-duplicating logic, but more consistent orderings is a nice-to-have.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D28017926
fbshipit-source-id: 5941588d0c8bba8b0df7d0084d5d198f4b7c2427
Summary:
While I think this was a very marginal bug with no known issues in the wild, incorrect layout values were sometimes being propagated to certain nodes. This would only occur during complex nested (un)flattening operations and may only impact node consistency, specifically with setting the "previous" ShadowView of mutation instructions, specifically REMOVE and DELETE. Even in rigorous testing I had trouble hitting this case and it didn't seem to impact the "next" values in CREATE, INSERT, or UPDATE.
The issue: previously `sliceChildShadowNodeViewPairsV2` assumed that the node it's operating on is a child of a non-flattened view, and the baseline origin is `{0,0}`. You can see when `sliceChildShadowNodeViewPairsRecursivelyV2` is called, a `layoutOffset` is passed in. If we ever got a list of a node that was in a flattened parent by calling `sliceChildShadowNodeViewPairsV2`, we would incorrectly assume that baseline layoutOffset for the node is `0,0`.
Now, we store the layoutOffset in the ShadowViewNodePair and can retrieve it when getting child pairs of a node.
Changelog: [internal]
Reviewed By: sammy-SC
Differential Revision: D27759380
fbshipit-source-id: a89756190a1cb377bcc55ff31799c2afbaecdaa9
Summary:
Previously, `ShadowViewNodePair::List` owned each `ShadowViewNodePair` but whenever we put `ShadowViewNodePair` into a TinyMap, those were unowned pointer references. This worked... 99% of the time. But in some marginal cases, it would cause dangling pointers, leading to difficult-to-track-down issues. So, I'm moving both of these to be unowned pointers and keeping a `std::deque` that owns all `ShadowViewNodePair`s and is itself owned by the main differ function. See comments for more implementation details. I'm moderately concerned about memory usage regressions, but practically speaking this will contain many items when a tree is created for the first time, and then very few items after that (space complexity should be similar to `O(n)` where `n` is the number of changed nodes after the last diff).
See comments as to why I believe `std::deque` is the right choice. Long-term there might be data-structures that are even more optimal, but std::deque has the right tradeoffs compared to other built-in STL structures like std::list and std::vector, and is probably better than std::forward_list too. Long-term we may want a custom data-structure that fits our needs exactly, but std::deque comes close and is possibly optimal.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27730952
fbshipit-source-id: 2194b535439bd309803a221188da5db75242005a
Summary:
I am fixing an extremely marginal case that probably impacts nothing in production, but in theory, could - see next diff in stack for the assert that is being hit.
TL;DR in marginal, complex cases with a lot of un/flattening, we can generate the following sequence of mutations:
```
UPDATE node V1 -> V2
REMOVE node V1
```
That is incorrect, and what we actually want is:
```
UPDATE node V1 -> V2
REMOVE node V2
```
While this, again, impacts /nothing/ in prod that we know of, it would be good to get this correct so that we can enable stricter asserts (see next diff).
This will also help with debugging LayoutAnimations.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D27697788
fbshipit-source-id: 47f34fe3e8107167b3df4db841d2cc14c58cb31d
Summary:
Allow conversion of LayoutMetrics to DebuggableString.
We also skipped a field in comparison. It probably isn't impactful in terms of production issues, but still wasn't correct.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27709451
fbshipit-source-id: 987fc2de0a4562a295d6cbeffdd922cbf056b811
Summary:
Turns out that ShadowViews that have different LayoutMetrics will have the same hash. Fix that.
This helps for debugging LayoutAnimations.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27585133
fbshipit-source-id: f0ac50619115150339089276e34fee5ddd0270bc
Summary:
See title. Not used in this diff; see next diff in stack.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D26001014
fbshipit-source-id: 475eb265f1c27bd9aff978a49652764a50287e47
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:
# 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:
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
Joshua Gross
Samuel Susla
David Vacca