Summary: That should help to fail early in situations when we lose critical class data members.
Reviewed By: sammy-SC
Differential Revision: D16179539
fbshipit-source-id: da73b81568c2f3657b9bc2bd1cc7ee6624e75626
Summary: This diff exposes LayoutDirection as part of UpdateLayoutMountItem
Reviewed By: JoshuaGross
Differential Revision: D16060521
fbshipit-source-id: 163bf2a0bdca62dcecb03a8aaa2f4bf595b18c8f
Summary:
Previously, we stored `keyIndex_` in a Parser object which is incorrect because it makes the parsing process thread-unsafe (the Parser is shared between parsing processes).
That worked previously most of the time because we never parsed props concurrently but we actually do this in the native animation library.
Reviewed By: yungsters
Differential Revision: D16064557
fbshipit-source-id: 211f4301d0746e0f5126a1dcdd59f1ae9a420aa9
Summary: In some cases, we cannot retrieve the "committed" state because no one state was mounted yet. The whole concept of "confirmed" or "legit at the moment" is kinda overstatement. The actual meaning of this is "the last vision of the state to which we advanced so far"; it does not have any relation to the actual "commit" phase or "mounting" process.
Reviewed By: sammy-SC
Differential Revision: D16002127
fbshipit-source-id: 95465e632525f873ae67f6db320a89562b62ba29
Summary: `Target` inside the Stage can be empty; in this case, we should not try to extract `ShadowNode` from it.
Reviewed By: JoshuaGross, mdvacca
Differential Revision: D15982479
fbshipit-source-id: 83a4bebadc88b59d7fe77acbdf07e8ce9f2f6be1
Summary: Returning a shared pointer by const reference in this context is not correct/safe because the object (the ShadowNode) doesn't own the object, so the caller cannot reason about the lifetime (esp. in a multithreaded environment).
Reviewed By: mdvacca
Differential Revision: D15958737
fbshipit-source-id: 8f03e6530d07d63ece5f955055c5c67c204b8223
Summary:
ComponentName is used by many core component of React Native, such as ComponentDescriptor, ShadowNode, ShadowView and so on. In all those cases this value represents the actual name of the component which came from `concreteComponentName` template parameter of ConcreteShadowNode. In all of those cases, it's raw `char const *` type. So, we don't need to use owning representation of the string (std::string) in all those places.
The only exception from this is a part where we receive the name of the component from JS side. In this case, the source string comes from JS and has to be analyzed as a character sequence to find corresponding ComponentDescriptor.
In my experiments, 20% of the time during diffing is spent on copying (this) `std::string`.
Reviewed By: mdvacca
Differential Revision: D15844407
fbshipit-source-id: a2e71505e22d09107e001bdf661d4a826bcf2dea
Summary:
This diff reimplements the prop parsing infrastructure in a part where it interacts with RawProps value.
Local synthetic tests show that the new way is 3x faster but the actual production result is quite unpredictable. MobileLab tests show some improvements about 10-20 ms on iPhone 6.
In short, the new way is faster because it inverts the lookup order and heavily relies on actual data types (and their properties) that we use. The old approach required about 130 hash-map lookups (where the key is `std::string`) to parse a single *Props object.
The new approach prepares concrete-props-specific tables with indexes of coming values ahead of time, iterates over raw data and puts it into those tables, and then performs a lookup in a very efficient manner.
Reviewed By: JoshuaGross
Differential Revision: D15752968
fbshipit-source-id: 847106e652eb7fc7ef7b99884a6f819ea3b9fd06
Summary:
In theory, those annotations can help the compiler to emit more optimized code and/or code that suggest CPU the proper way to utilize the instruction pipeline after the coming branch.
TBH, it's not clear how exactly beneficial those annotations are (esp. on target architectures) but they certainly don't hurt (in all cases here the favorite code branch is obvious).
Reviewed By: mdvacca
Differential Revision: D15752969
fbshipit-source-id: 8adf25a48107ffde828f735fb1386b30dbe63ede
Summary:
Previously we used `std::string` as a type behind all prop names (and name fragments). Even if `std::string` converted from `char const *` should be heavily optimized by STL and compiler, we still concatenate and copy those strings a lot. Switching to `char const *` allows avoiding tons of copying and inefficient equality checks.
Besides that, the future, more sophisticated optimization will rely on this.
Reviewed By: mdvacca
Differential Revision: D15511493
fbshipit-source-id: 9f509d18f0c737f7f77d4fea192d2ed1872e3731
Summary: This is a small perf tweak that alone does not give much improvement but the coming diffs will rely on the possibility to construct empty RawProps object.
Reviewed By: mdvacca
Differential Revision: D15511494
fbshipit-source-id: 39dec9336e6b5cf6ad33b1f3a06ca1c096ece628
Summary:
And, btw, the tests show that performance of that is not so great:
```
Running /Users/shergin/fbsource/fbobjc/buck-out/cells/fbsource/gen/xplat/js/react-native-github/ReactCommon/fabric/core/benchmarks
Run on (12 X 2900 MHz CPU s)
CPU Caches:
L1 Data 32K (x6)
L1 Instruction 32K (x6)
L2 Unified 262K (x6)
L3 Unified 12582K (x1)
--------------------------------------------------------------------------------------------
Benchmark Time CPU Iterations
--------------------------------------------------------------------------------------------
propParsingUsingComponentDescriptor 79630 ns 77991 ns 8864
propParsingUsingComponentDescriptorWithNoSourceProps 70200 ns 69099 ns 8362
```
Which means 70ms per 1000 prop parsing processes.
Reviewed By: JoshuaGross, mdvacca
Differential Revision: D15608677
fbshipit-source-id: ed4feca489e1243adc73de4741c287256c3aaec3
Summary:
First of all, seems it's the right thing to do. Fabric C++ code is cross-platfrom and should run on *all* platforms including Windows, Linux, and Mac.
While we don't have a real *production* use cases where we need compilation for desktops, having CXX target is really handy for two reasons:
* It simplifies local test running process. Instead of going to `/fbandroid/` and executing something like `buck test fbsource//xplat/js/react-native-github/ReactCommon/fabric/core:coreAndroid` (note the suffix). We can just do `buck test fbsource//xplat/js/react-native-github/ReactCommon/fabric/core:core` everywhere and it works now out of the box. Running tests with "Apple" flavor never worked for me.
* It allows creating synthetic benchmark tests (using Google Benchmark) that can be used as a rough approximation of code micro-optimizations.
Reviewed By: JoshuaGross
Differential Revision: D15608678
fbshipit-source-id: d2449035685dbca6ab983480f5334ec4ac11cd35
Summary:
We are not sure yet how exactly this should work semantically (e.g. should unbatched events flash previously dispatched batched or not).
So, let's disable that until we have all answers.
Reviewed By: mdvacca
Differential Revision: D15498191
fbshipit-source-id: 77f07c5e86bfbfd212505df8cc6530e39531b5ef
Summary:
This diff restores the original role of StateData as a dummy class that has only one purpose - designate that there is no state associated with the node.
I believe having an abstract StateData class for all data types is not necessary and actually slightly harmful. And here the reasons:
* Having virtual dispatch enabled for classes introduces some overhead, and it should be used only if it is absolutely necessary. In this case, we don't need to have virtual dispatch enabled for Data classes because it's already enabled for State classes; therefore all virtual resolution is done there and that's sufficient. No need to pay for what we don't need.
* Continuing the previous point, yes, we expect some API being exposed for Data classes (such as `getDynamic`), but introducing a base abstract class for that is *not* an idiomatic C++ solution; in C++ it's by design that most of the template contracts are actually duck-typed. This is a well-known design issue/concern that will be addressed with an effort called "Concepts" in C++20. Anyway, we should not use abstract classes as *only* indication of conformance to some interface.
* StateData does not introduce any convenience. As it is clear from several subclass implementations, we don't really use base methods from it.
* The previous issue actually happens especially because of the interface of StateData class. Consider constructor that accepts `folly::dynamic` and does nothing, it actually useless because it's impossible to use it unless it's called inside superclass's constructor. That's the case because C++ does not support virtual dispatch for constructors (for good).
* Avoiding subclassing StateData counter-intuitively enforces the desired conformance to imaginary interface: it does not compile otherwise.
Reviewed By: JoshuaGross
Differential Revision: D15342338
fbshipit-source-id: 1f04f7fc5247499e7cfc09cd4b3cccffdc0b6b06
Summary: A couple of new methods in ConcreteShadowNode allows us to deal with State in more LocalData-like manner.
Reviewed By: mdvacca
Differential Revision: D15323686
fbshipit-source-id: ede4aa1f1d0ad6f876bd963e57a00a0ad470c1c0
Summary:
Previously the pointScaleFactor field was not being compared properly in LayoutMetrics equality method.
This diff fixes that
Reviewed By: shergin
Differential Revision: D15303555
fbshipit-source-id: 8863e9e1fbad15b43400afc32b97bf6d252cbe55
Summary: I don't recall why the mutation function return rvalue reference, but it is totally incorrect (the function cannot own the object, so returning a reference introducing a dungling pointer and will crash).
Reviewed By: mdvacca
Differential Revision: D15156312
fbshipit-source-id: 497d10de22a41906efe71cd10139e3710ae11a79
Summary: After this change, all measuring operations based on `LayoutableShadowNode::getRelativeLayoutMetrics` will take into account the transformation matrices provided by shadow nodes. This will allow implementing scroll-offset-aware and custom-transform-aware measuring.
Reviewed By: JoshuaGross, mdvacca
Differential Revision: D15219259
fbshipit-source-id: 1d7cd8c0ee4406f4dd0002cd442dc0679391922b
Summary:
`LayoutableShadowNode::getTransform` returns a transform object that represents transformations that will/should
be applied on top of regular layout metrics by mounting layer.
Reviewed By: mdvacca
Differential Revision: D15219262
fbshipit-source-id: e7aeb85b5f7e2fce3f8faf9dfcaee5dae3217d36
Summary:
Previously we computed the list of nodes that need to be notified about layout changes using a list of mutation instructions. That was fine, but that's not really compatible with some other changes that I plan to make, so I decided to change it (make it better).
Besides the better design (debatable; fewer dependencies to unrelated moving pieces), here is why I believe the new way is more performant:
* The new approach has no `dynamic_casts`, whereas the previous has tons of them (two per a mutation). If a `dynamic_cast` takes 10 ns, for 500 nodes it can take up to 5ms only for casts. (Non-scientific assumption.)
* After removing dependency to mutation instruction, we can enable flattening for views which have `onLayout` event.
Reviewed By: mdvacca
Differential Revision: D15110725
fbshipit-source-id: 31a657ccfd02441734ad1d71a833653223163289
Summary: `kFloatUndefined` means "no value here", but in this particular case, we have to have `Infinity` value that represents maximum available space.
Reviewed By: mdvacca
Differential Revision: D15155190
fbshipit-source-id: d2de20681ad04da7444331eff44b93d2bd0200e3
Summary: It turns out that just only props is not enought to build an initial state value in some cases for some component. Seems we need at least `surfaceId` and `eventEmitter` in some cases (which seems totally reasonable). So, seems using the whole `ShadowNodeFragment` for that purpose is a good choise.
Reviewed By: mdvacca
Differential Revision: D15039135
fbshipit-source-id: d9a5f47f971ccf6cdb2f888bd31f7948b37b67ef
Summary:
`ShadowNodeFragment` is very cheap by design because it does not own stuff it contains, so it's great. But... sometimes we need to own the stuff (e.g. to pass it on the other thread), in those cases we can use `ShadowNodeFragment::Value` now.
`ShadowNodeFragment::Value` cannot be used alone, it needs to be constructed from `ShadowNodeFragment` and then used as opaque object and then it can be converted to ``ShadowNodeFragment`.
We will need it soon.
Reviewed By: mdvacca
Differential Revision: D15039136
fbshipit-source-id: d40875cac05f4088358d8d418007d17df9ff14f4
Summary:
Trivial.
We are replacing rootTag with surfaceId according to the plan describing here: https://fb.workplace.com/groups/rn.fabric/permalink/1374002366064519/
Reviewed By: JoshuaGross, mdvacca
Differential Revision: D15039134
fbshipit-source-id: ec8c3044f9f3f23939488bc01c66e9b653e651dd
Summary:
@public
In order to encapsulate property access on `YGStyle`, as a first measure we wrap all fields with accessors.
This will e.g. enable dynamic property storage and instrumentation in the future.
All accessors have a `const` version that allows direct access via `const&`. For mutation, bit fields are wrapped with a custom reference object.
This style allows for the least amount of changes in client code. Property access simply needs appended parens, eg `style.direction` becomes `style.direction`.
Reviewed By: shergin
Differential Revision: D14999096
fbshipit-source-id: fbf29f7ddab520513d4618f5e70094c4f6330b30
Summary: `getContextContainer` should be marked as const so that const instances can call it.
Reviewed By: shergin
Differential Revision: D14969981
fbshipit-source-id: 8812f24ecf0642a38496580689943fbd43cddad1
Summary:
Registries, providers, providers of registries, registres of providers. All that can be really confusing, but that represents best the constraints and desires that we have:
* We need to be able to register components on-the-fly (so we need a mechanism that will propagate changes);
* We don't want to register ComponentDescriptors separately from ComponentView classes;
* C++ not always gives us abstractions that we want (e.g. pointers to constructors).
After the change, we can remove the whole Buck target that has a bunch of handwritten boilerplate code.
There is a still room for polish and removing some concepts, types or classes but this diff is already huge.
Reviewed By: JoshuaGross
Differential Revision: D14983906
fbshipit-source-id: ce536ebea0c905059c7a4d644dc25233e2809761
Summary:
ComponentDescriptorProvider represents unified way to create a particular descriptor.
Now all ComponentViews (which support RCTComponentViewProtocol) expose a `ComponentDescriptorProvider` which will allow creating and registering ComponentDescriptor instances for all visual components automatically as a part of ComponentView registration process.
Don't panic, everything is still being as explicit as it always was, no magic involved; we just will have only one registration step instead of two parallel.
That also opens a way to register components on the fly.
Reviewed By: JoshuaGross
Differential Revision: D14963488
fbshipit-source-id: 9e9d9166fabaf7b30b35b8647faa6e3a19cd2435
Summary:
Turns out that storing and using ContextContainer in custom subclasses is a huge pain. At the same time seems that a lot of custom components need some DI instrument, so we need this instrument anyway.
Moving stuff from the template to the base class should also help with codesize a bit.
Reviewed By: JoshuaGross
Differential Revision: D14921356
fbshipit-source-id: 4dbb961fe32bd66c73513d7e053bbed229860a31
Summary:
Previously, all placeholders methods have return type `SomeType &` which is not correct because it allows the called to modify enclosed `static` value of the placeholders; the type should be `SomeType const &`.
Besides that this diff migrates some type aliases to the new style (which makes evething much prettier and readable).
Reviewed By: mdvacca
Differential Revision: D14819076
fbshipit-source-id: 87e68d546f16d7a9ad1fb65e1b238859f9381eb7
Summary: Small changes to State objects to support Android. See following diffs.
Reviewed By: mdvacca
Differential Revision: D14663470
fbshipit-source-id: 878f4dc39265991a7b8ff54ca80bdb862f1dd3de
Summary: Same algorithm using a new API.
Reviewed By: JoshuaGross
Differential Revision: D14423509
fbshipit-source-id: c40f61fdd001f68785512c304941f523585d641c
Summary:
The algorithm is quite simply:
1. Get family objects of the two nodes (inner and outer).
2. Traverse the list of family objects starting from the inner one and form a reversed list of them.
3. tarting from the inner node, traverse the tree layer-by-layer choosing a next element of the path by comparing the family object of each node on the level and an object from the list.
Reviewed By: JoshuaGross
Differential Revision: D14416950
fbshipit-source-id: 23c659a9e01690f90174193650a2b0ef09eadb4d
Summary:
One of the core feature of ShadowNodeFamily is having a pointer to a parent family. This diff implements it.
I don't think there is a hard retain cycle there, but for more lean memory usage we use weak_ptr here.
Reviewed By: JoshuaGross
Differential Revision: D14416948
fbshipit-source-id: 05fd2c4833146f007228363b1d958776b4a2a9cf
Summary:
ShadowNodeFamily is a new concept that allows us to implement an efficient way to solve the problem of finding a path to some ancestor node (practically reimplement ShadowNode::constructAncestorPath() in some efficient way).
This diff is the first one in the series. It introduces a new class and moves some data into it.
Reviewed By: JoshuaGross
Differential Revision: D14416947
fbshipit-source-id: c93865a8929a2128498e34d3589487696aac6283
Summary: This is a leftover from the ealy days of Fabric. The mehtod does not have implementation counterpart and any usage.
Reviewed By: JoshuaGross
Differential Revision: D14402636
fbshipit-source-id: aa2e919084ae7382d3a62af761e1f6eac334fc75
Summary: In React, things like changing `tag`, `eventTarget` or reparenting are impossible by design. Which seems like a strange limitation actually allows us to do tremendous performance optimizations (stay tuned!).
Reviewed By: JoshuaGross
Differential Revision: D14402638
fbshipit-source-id: 3b7b7edaff0d55a3ca94e2ac4c753d630d07101d
Summary: In methods which implement move semantic we have to check the source object because it's (also) being mutated.
Reviewed By: JoshuaGross
Differential Revision: D14496937
fbshipit-source-id: f3f2299d14e41c8b269f1647336dbd5b45c235f4
Summary:
If the `HasNewLayout` flag is `false`, we should not copy the data from YGStyle/YGLayout to ShadowNode because the node can be already sealed and because it's unnecessary. Previously we workaround this case with a special check in `setLayoutMetrics` method, but that can be unreliable because of some side-effects related to pixel rounding and comparing floats.
The new approach is much more robust and explicit.
Reviewed By: JoshuaGross
Differential Revision: D14496939
fbshipit-source-id: deddb14d2206c5bd3f22154d0ea682e3c5888901
Summary: These default implementations are never being used. Removing them allowing to ensure that flags inside YGNode have same values as flags in YogaLayoutableShadowNode. That also saves a couple of bytes in size of ShadowNode.
Reviewed By: JoshuaGross
Differential Revision: D14496938
fbshipit-source-id: c43f9c8a2eec054f728ff54a6573668eccda55fb
Summary: Several things need to ironed out: 1) LocalState in Fabric C++, 2) setting dimensions of BottomSheet component to 0,0 for parent.
Reviewed By: shergin
Differential Revision: D14426167
fbshipit-source-id: 45a90a7971c87672872108a9e360926b4a6095f0
Summary:
Accidentally I noticed that the signature of Folly's hash_combine is different from boost's one.
The Folly's one is:
`size_t hash_combine(const T& t, const Ts&... ts)`, so the first argument is immutable and the method returns the result normally.
It means that all hashes that we compute in Fabric using `hash_combine` were `0`.
So I fixed it.
I have no idea why this difference exists, but some modern papers suggest that folly's variant has good chances to be standardized.
E.g.: http://open-std.org/JTC1/SC22/WG21/docs/papers/2017/p0814r0.pdf
Technically, it should improve performance, but I doubt that it can be more than 1-2 ms per screen TTI.
Reviewed By: JoshuaGross
Differential Revision: D14430380
fbshipit-source-id: 97da999ee5780b940bb789bc3eb5bf9f89c194ca
Summary: The hope is that it will remove many unnececery allocations improving overal perfromance.
Reviewed By: mdvacca
Differential Revision: D14249198
fbshipit-source-id: f0442b3919ccead0582a3190dea0e33d517d85f6
Summary:
SharedShadowNodeList is one of the core collections in Fabric. Every ShadowNode has one, it's used intensivly during diffing and so on.
Usually, nodes have a very few children, so using `small_vector` instead of regular `vector` should save us a lot of memory allocations.
Reviewed By: JoshuaGross
Differential Revision: D14249201
fbshipit-source-id: 53297caa027a74099d0a1ed4a3cce78f8feb651b
Summary:
Nowadays, every ShadowNode has a reference to a ComponentDescriptor, so now there is no need to do registry lookup for that.
That should save us 0.5-1.0 ms in my non-scientific measurements.
Reviewed By: mdvacca
Differential Revision: D14348369
fbshipit-source-id: 57f6a6f2f8bf0b7e58d89a414fec20b2db8876f7
Summary:
This pull request removes the designated initializer in `LayoutMetrics.h`. This will help improve the portability of this file.
[General] [Changed] - Fabric: Remove designated initializer in LayoutMetrics.h
Pull Request resolved: https://github.com/facebook/react-native/pull/23758
Differential Revision: D14320526
Pulled By: shergin
fbshipit-source-id: 076ad389d0985d5213b521a2ffaca4ca7ae31599
Summary:
This pull request removes the use of the GCC extension null coalescing operators (`?:`) and replaces them with ternary operators. This improves the portability of `ShadowNode.cpp` (and enables MSVC to build it)
[General] [Fixed] - Fabric: Removed null coalescing operators in `ShadowNode`
Pull Request resolved: https://github.com/facebook/react-native/pull/23438
Differential Revision: D14304958
Pulled By: shergin
fbshipit-source-id: 7d8e6778a72dabf09b1d99bc091a7578598b79c3
Min ho Kim
Valentin Shergin
David Vacca
David Aurelio
Joshua Gross
empyrical