Summary:
Changelog: [internal]
Reland of D29131766 (https://github.com/facebook/react-native/commit/18165367b0347fc46cd52a6ac00afcf62d05cb30) which had to reverted because it caused binary size regression in instagram.
Size check for `automation_instagram_stablesize_release` and `automation_igtv_release`
{F626711916}
Reviewed By: JoshuaGross
Differential Revision: D29263302
fbshipit-source-id: cc8f5609ebaed9ddf666f7c57cdbf3dbf77a8f78
Summary:
Because of T92179998, T93607943, and T93394807, we are still seeking resolution to tricky crashes wrt the use of EventEmitters.
I believe the recent spike is because of two recent changes: we pass in EventEmitters earlier, during PreAllocation; and we clean them up earlier, during stopSurface, to avoid jsi::~Pointer crashes.
Additionally, the gating previously added around the PreAllocation path was incorrect and led to more nullptrs being passed around as EventEmitters.
To mitigate these issues:
1) I am adding/fixing gating to preallocation and early cleanup paths
2) I am making EventEmitterWrapper more resilient by ensuring EventEmitter is non-null before invoking it.
3) I am making sure that in more cases, we pass a non-null EventEmitter pointer to Java.
4) I am backing out the synchronization in EventEmitterWrapper (java side) as that did not resolve the issue and is a pessimisation
There are older, unchanged paths that could still be passing in nullptr as the EventEmitter (Update and Create). As those have not changed recently, I'm not going to fix those cases and instead, we can now rely on the caller to ensure that the EventEmitter is non-null before calling.
Changelog: [internal]
Differential Revision: D29252806
fbshipit-source-id: 5c68d95fa2465afe45e0083a0685c8c1abf31619
Summary:
Changelog: [internal]
RuntimeScheduler needs to be created and registered in the runtime before any JS is allowed to run. This diff moves the registration right after the runtime is initialised.
This diff removes funnelling of Fabric events through RuntimeScheduler. This will be added in subsequent diff to keep the complexity low.
Reviewed By: JoshuaGross
Differential Revision: D29131766
fbshipit-source-id: cbc650f6fbce95e4b9c2c9695e8e0aba5beac635
Summary:
setJSResponder/clearJSResponder have been in use in prod for a while and are stable. Ship them in code.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D28889894
fbshipit-source-id: 1c42526cd890d528062eeb50761fc49cc6109d76
Summary:
The RuntimeExecutor that Fabric gets from the bridge doesn't call JSIExecutor::flush(). In the legacy NativeModule system, we're supposed to flush the queue of NativeModule calls after every call into JavaScript. The lack of this flushing means that we execute NativeModule calls less frequently with Fabric enabled, and TurboModules disabled. It also means that [the microtask checkpoints we placed inside JSIExecutor::flush()](https://www.internalfb.com/code/fbsource/[62f69606ae81530f7d6f0cba8466ac604934c901]/xplat/js/react-native-github/ReactCommon/jsiexecutor/jsireact/JSIExecutor.cpp?lines=427%2C445) won't be executed as frequently, with Fabric enabled.
Changelog: [Android][Fixed] - Flush NativeModule calls with Fabric on Android, on every Native -> JS call.
Reviewed By: JoshuaGross, mdvacca
Differential Revision: D28620982
fbshipit-source-id: ae4d1c16c62b6d4a5089e63104ad97f4ed44c440
Summary:
Removes stale feature flag that was in production for a couple of months. Fix helped to decrease number of crashes significantly, so we can remove it.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D28757995
fbshipit-source-id: 375da09c11f265e8bbe03cd99de1b83f168420ce
Summary:
EventDispatcherImpl uses synchronized blocks all over to make it thread-safe. I'm concerned about the perf implications of this and creating contention between JS and UI threads.
This is locked behind a feature flag.
Enabled only for Fabric in StaticViewConfig mode, and a feature flag, for now.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D28591331
fbshipit-source-id: ea8f93a2e1343ce37fa78690dcb62fe03594120f
Summary:
FlatList relies heavily on onScroll events + the measure API. In Fabric, usage of `measure` relies on C++ having an accurate view of the current scroll position of the ScrollView.
We already have a mechanism for updating the scroll position in C++ using UpdateState. But, it is only used currently at the /beginning/ and /end/ of scrolling, and UpdateState is not called /during/ scrolling at all.
This means that we will see a series of events like this while scrolling:
```
Scrolling begins
UPDATE C++ STATE: scrollLeft = 0, scrollTop = 0
JS event: onScroll x=0, y=0
JS event: onScroll x=0, y=100
JS event: onScroll x=0, y=200
...
JS event: onScroll x=0, y=1000
UPDATE C++ STATE: scrollLeft = 0, scrollTop = 1000
```
Notably, not many C++ state updates are queued; and the last one is queued AFTER the JS event is sent. The last JS event and UpdateState will race, which means that sometimes the C++ update will /lose/ and C++ will have an inaccurate view of the world when FlatList receives the onScroll event and calls `measure`.
My proposed solution, gated behind a feature flag, is to delay /some/ onScroll events until the C++ UpdateState has made its way back to Java, and send UpdateStates more frequently. The balance here is that UpdateState is a relatively expensive operation, so we probably still want to call it /less/ than we call onScroll. This means that `measure` will still return some incorrect results but will return correct results more frequently. Win?
Changelog: [Internal[
Reviewed By: mdvacca
Differential Revision: D28558380
fbshipit-source-id: 11c7cd714fae67ee5a94c4413be988481413ec03
Summary:
This diff creates a ReactFeatureFlag to initialize MapBufferSo file during Fabric initialization.
This is necessary to be able to compare properly Mapbuffer vs ReadableNativeMap (because ReadableNativeMap c++ files is already included in the bridge so file)
changelog: [internal]
Reviewed By: JoshuaGross
Differential Revision: D28436044
fbshipit-source-id: 338e1bb72b5313dc29a309e1b0e979e7c8bd1c18
Summary:
This diff is a follow up of D28360679 (https://github.com/facebook/react-native/commit/e3367354cc93f17b830ed8dc601dff5e87748dd1), here we refactor the access of ReactFeatureFlags from C++ to use methods instead of fields
changelog: [internal] internal
Reviewed By: JoshuaGross
Differential Revision: D28362066
fbshipit-source-id: caed5e7fddeb6c0d9846fb037152befa8f1ed5c2
Summary:
Since we are now using ReactFeatureFlag from C++, we need to ensure redex doesn't strip its fields.
changelog: [internal] internal
Reviewed By: JoshuaGross
Differential Revision: D28360678
fbshipit-source-id: 74604e2d008a056c161d8b6ab8f5b30807087d9e
Summary:
This diff deletes ReactFeatureFlags.useViewManagerDelegatesForCommands, this has been enabled in prod for 9+ months
changelog: [internal] internal
Reviewed By: JoshuaGross
Differential Revision: D28265338
fbshipit-source-id: 2f07cb83d6ef9191f9ebea52e230490ef98d6e2d
Summary:
This diff deleted the ReactFeatureFlags.useViewManagerDelegates, this has been enabled for 9+ months
changelog: [internal] internal
Reviewed By: JoshuaGross
Differential Revision: D28265339
fbshipit-source-id: f5c97e77ca4fc72d2e2b8f891e800e362177d67a
Summary:
This diff creates a MC to verify impact of eager initialization of fabric classes, the purpose is to remove this code, but before doing that I would like to verify what's the impact.
changelog: [internal] internal
Reviewed By: sammy-SC
Differential Revision: D28223943
fbshipit-source-id: 6f7c4701fb730fe1c0629ec13ead592ff619373f
Summary:
With D27975839, RuntimeExecutor will be able to start flushing the queued up NativeModule calls in every call from C++ -> JavaScript. We're going to test this feature to measure its impact. In this diff, I wire up the the MobileConfig to ReactFeatureFlags.
Changelog: [Internal]
Reviewed By: JoshuaGross
Differential Revision: D27978112
fbshipit-source-id: 47e1e74398c62755bb0fdc6b54b7fd3aa47eb877
Summary:
JSI callbacks are only destroyed if the callback is called. If the callback is never called, we're potentially leaking a lot of callbacks.
To mitigate this, we add a wrapper object that is owned by the std::function. Whenever the std::function is destroyed, the wrapper is destroyed and it deallocates the callback as well.
Changelog: [Internal]
Reviewed By: RSNara
Differential Revision: D27436402
fbshipit-source-id: d153640d5d7988c7fadaf2cb332ec00dadd0689a
Summary:
D26581756 (https://github.com/facebook/react-native/commit/86321a35c03df340b6c1ea740ea7f29e2659b0ab) was hacked for fixing T85822390, which was later properly fixed in D27369721, so I removed it in this diff.
Changelog:
[Android][Fixed] - Remove previous fix for "Fix text in ReactTextView sometimes being vertically displayed"
Reviewed By: mdvacca
Differential Revision: D27501405
fbshipit-source-id: 81417069d355936721868ce659b3fe8ce916302e
Summary:
This diff contains the code from the 35 diff stack - D27210587
This diff implement and integrates Mapbuffer into Fabric text measure system
changelog: [internal] internal
Reviewed By: JoshuaGross
Differential Revision: D27241836
fbshipit-source-id: f40a780df0723f27da440f709a8676cfcca63953
Summary:
Create MC to gate execution of JS Responder in Fabric Android
MC.react_fabric.enable_js_responder_fabric_android is enabled by default in the server
changelog: [internal] internal
Reviewed By: JoshuaGross
Differential Revision: D26905296
fbshipit-source-id: 82504174394d1e10fd017435cccd38952404fda0
Summary:
## Issue:
Sometimes ReactTextView are vertically displayed as one column with bridgeless.
## Root cause:
After debugging, I found out this is caused by a workaround in 2016 to fix a crash caused by mLayout occasionally being non-null and triggers relayout during setText.
https://github.com/facebook/react-native/pull/7011
## Fix
Revert previous hack, if the crash happens again I'll try to fix it.
Changelog:
[Android][Fixed] - Fix text in ReactTextView sometimes being vertically displayed
Reviewed By: mdvacca
Differential Revision: D26581756
fbshipit-source-id: a373d84dc1ab3d787bda7ec82f2d0865a354cf60
Summary:
This bug doesn't reproduce anymore in v301+. MC has been enabled since december
https://www.internalfb.com/intern/logview/details/facebook_android_javascripterrors/419f8892e7b1a02f205810219ddfc299/trends?selected-logview-tab=All%20Traces&drillstate={%22start%22:%22Thu,%2028%20Jan%202021%2000:59:54%20-0800%22,%22end%22:%22Thu,%2011%20Feb%202021%2000:59:54%20-0800%22,%22constraints%22:[{%22col%22:%22mid%22,%22op%22:%22==%22,%22vals%22:[%22419f8892e7b1a02f205810219ddfc299%22]}],%22context%22:%22facebook_android_javascripterrors%22,%22metric%22:%22count%22}
changelog: [internal] internal
Reviewed By: ShikaSD
Differential Revision: D26398484
fbshipit-source-id: ca85ca211f1a38aa2691f150956a27c878d243bc
Summary:
This diff cleans listeners on the destruction of the ReactContext.
changelog: [inernal] internal
Reviewed By: JoshuaGross
Differential Revision: D26259929
fbshipit-source-id: 1843cabdac2fa3e67dcc890afd923b82472d8f66
Summary:
## Context
Every time we require a NativeModule in Java, we [first try to create it with the TurboModuleManager](https://fburl.com/diffusion/3nkjwea2). In the TurboModule infra, when a NativeModule is requested, [we first create it](https://fburl.com/diffusion/d2c6iout), then [if it's not a TurboModule, we discard the newly created object](https://fburl.com/diffusion/44gjlo6y). This is extremely wasteful, especially when a NativeModule is requested frequently and periodically, like UIManagerModule.
Therefore, in D24811838 (https://github.com/facebook/react-native/commit/803a26cb003e6b790e3a1ab31beb0c95795fff0c) fkgozali launched a fix to the infra that would avoid creating the non-TurboModule object in the first place. Today, we're launching this optimization.
Reviewed By: fkgozali
Differential Revision: D25621570
fbshipit-source-id: dedba4d5ac6fcf2ec3c31e7163a6a226065c708b
Summary:
The flag `clipChildRectsIfOverflowIsHidden` has been set to false for a little over a year. Delete it and associated callsites.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D25451696
fbshipit-source-id: a6067b2e25124f6bdef336c2ddead719dd44cca9
Summary:
In practice this has been enabled in production for months and is fine. Remove feature-flag and gating.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D25451697
fbshipit-source-id: 9e70db3ed4e00de7b8295d9225d43ba09e4e68e9
Summary:
This has been true for 100% of users for months; clean up the old code.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D25451470
fbshipit-source-id: feae59ce746869b9d84d6aaa69be10e91181f03a
Summary:
This code and this class isn't needed in Fabric, so don't create it during construction. Lazy-load it when it's needed outside of Fabric.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D25450724
fbshipit-source-id: d14d8f03c194716f2aba0e499f3282ad2d1c1d29
Summary:
This diff prevents the pre-calculation of ViewManager's constants for users with static view config enabled.
We still load viewManager classes and create viewManger objects for perf reasons
Changelog: [Internal]
Reviewed By: fkgozali
Differential Revision: D25414068
fbshipit-source-id: a91f6113e35b42625c03d13bd67b63e3f9f75098
Summary:
Props are transferred from C++ to Java using ReadableNativeMaps. The current implementation of ReadableNativeMaps creates an internal HashMap the first time one of its methods is executed.
During the update of props ReadableNativeMaps are consumed only once and they are accessed through an Iterator. That's why there is no reason to create the internal HashMap, which is inefficient from performance and memory point of view.
This diff creates an experiment that avoids the creation of the internal HashMap during prop updates, additionally it removes a lock that was being used in the creation of the internal HashMap. I expect this change to have a positive impact in TTRC and memory (in particular for ME devices)
This diff reduces the amount of ReadableNativeMaps's internal HashMaps created during initial render of MP Home by ~35%.
Changelog: [Internal]
Reviewed By: JoshuaGross
Differential Revision: D25361169
fbshipit-source-id: 7b6efda11922d56127131494ca39b5cec75f1703
Summary:
The root cause of this bug is a race condition between the onMeasure method and setupReactContext.
ReactInstanceManager.attachRootView() method is responsible of the initialization of ReactRootViews and it is invoked by ReactRootView.onMeasure() method in the UIThread
Important initialization steps:
1. Clear the Id of the ReactRootView
2. Add the ReactRootView to the mAttachedReactRoots
3. Call StartSurface (if the bridge has been initialized)
Sometimes, when this method is invoked for the first time, the bridge is not initialized, in those cases we delay the start of the surface.
Once the bridge is initialized, StartSurface is called by the setupReactContext() running in the NativeModuleThread.
Since onMeasure can be called multiple times, it is possible that we call "StartSurface" twice for the same ReactRootView, causing the bug reported on T78832286.
This diff adds an extra check to prevent calling "StartSurface" twice. The fix is done using an AtomicInteger comparison and it is gated by the flag "enableStartSurfaceRaceConditionFix". Once we verify this works fine in production we will clean up the code, remove the flags and maybe revisit the API of ReactRoot.
changelog: [Android] Fix race-condition on the initialization of ReactRootViews
Reviewed By: JoshuaGross
Differential Revision: D25255877
fbshipit-source-id: ca8fb00f50e86891fb4c5a06240177cc1a0186d9
Summary:
This feature flag (enableStopSurfaceOnRootViewUnmount) was used to gate usage of the "stopSurface" API, which is now fully supported, and has been used
in the FB app for several months. This is safe to ship in code.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D25275192
fbshipit-source-id: fa22bfd00aa023297bc19c83c138f133e9ff1645
Summary:
This was added to prevent mutating the UI during draw or measure, and appears to have been effective. Keep the comments, ship the feature, remove flags.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D25258409
fbshipit-source-id: 36ad8a03d1eb82bc9dcd769372c03f1ebe8b8da8
Summary:
Currently, queuing any sort of MountItem or getting the list of MountItems requires synchronization. Since these can be queued up at any point from the JS thread, there will, in theory, be constant contention between JS and UI thread.
To see if this is true, I'm creating an experiment instead of just switching over to concurrent structures. After seeing results, we can hopefully make a decision and delete the non-concurrent stuff or improve on it somehow.
The original was unlanded in D24973616 (https://github.com/facebook/react-native/commit/26787e2260412d9d2fe831e68a8616505d3cab36) due to a typo, which has been fixed now. The typo was that in Blocking Mode, we queued up all PreMountItems to the concurrent PreMountItems queue.
Changelog: [Internal]
Reviewed By: ShikaSD
Differential Revision: D24974851
fbshipit-source-id: d081c081bbd0de445bb92408f0ec822056b905a5
Summary:
Currently, queuing any sort of MountItem or getting the list of MountItems requires synchronization. Since these can be queued up at any point from the JS thread, there will, in theory, be constant contention between JS and UI thread.
To see if this is true, I'm creating an experiment instead of just switching over to concurrent structures. After seeing results, we can hopefully make a decision and delete the non-concurrent stuff or improve on it somehow.
Changelog: [Internal]
Reviewed By: ejanzer
Differential Revision: D24942110
fbshipit-source-id: fcbdeda51f91f4bd430a20d7484854fb1e94a832
Summary:
There is a flow where TM registry is creating a module instance (as registered in the TurboReactPackage), only to discard it if it's not a TM enabled module. This may be fine for many modules, but for module like `UIManagerModule`, this may cause a race condition or other issues, including potential perf regression when accessing UIManager from JS (e.g. for getting native viewConfigs).
Changelog: [Internal]
Reviewed By: RSNara
Differential Revision: D24811838
fbshipit-source-id: 6e1cce6993a6e5c9763773f175083bf52925c910
Summary:
## Android API
```
// Before we initialize TurboModuleManager
ReactFeatureFlags.useTurboModuleJSCodegen = true
```
## iOS API
```
// Before we initialize RCTBridge
RCTEnableTurboModuleJSCodegen(true);
```
## How is the JS Codegen actually enabled?
The above native flags are translated to the following global variable in JavaScript:
```
global.RN$JSTurboModuleCodegenEnabled = true;
```
Then, all our NativeModule specs are transpiled to contain this logic:
```
interface Foo extends TurboModule {
// ...
}
function __getModuleSchema() {
if (!global.RN$JSTurboModuleCodegenEnabled) {
return undefined;
}
// Return the schema of this spec.
return {...};
}
export default TurboModuleRegistry.get<Foo>('foo', __getModuleSchema());
```
Then, in our C++ JavaTurboModule, and ObjCTurboModule classes, we use the TurboModule JS codegen when the jsi::Object schema is provided from JavaScript in the TurboModuleRegistry.get call.
Changelog: [Internal]
Reviewed By: PeteTheHeat
Differential Revision: D24636307
fbshipit-source-id: 80dcd604cc1121b8a69df875bbfc87e9bb8e4814
Summary:
TurboModule methods that return promises are synchronously run on the JavaScript thread. Back in D22489338 (https://github.com/facebook/react-native/commit/9c35b5b8c4710dfe6a4b689a5565aa78ae5b37d3), we wrote code to make them dispatch on the NativeModules thread. That code, however, was just left disabled. In this diff, I wire up the TurboModules infra to a MobileConfig, which should allow us to assess the performance impact of async dispatch of promise methods to the NativeModules thread in production, before we roll it out more widely.
Changelog: [Internal]
NOTE: This diff was reverted, beacuse we landed it it without D24685387.
Reviewed By: ejanzer
Differential Revision: D24787573
fbshipit-source-id: 324bd22ce79c2c16c7f7b6996496d255a2c6256e
Summary:
TurboModule methods that return promises are synchronously run on the JavaScript thread. Back in D22489338 (https://github.com/facebook/react-native/commit/9c35b5b8c4710dfe6a4b689a5565aa78ae5b37d3), we wrote code to make them dispatch on the NativeModules thread. That code, however, was just left disabled. In this diff, I wire up the TurboModules infra to a MobileConfig, which should allow us to assess the performance impact of async dispatch of promise methods to the NativeModules thread in production, before we roll it out more widely.
Changelog: [Internal]
Reviewed By: fkgozali
Differential Revision: D24685389
fbshipit-source-id: 8ceb2e6effc125abecfa76e5e90bd310676aefc9
Samuel Susla
Joshua Gross
Ramanpreet Nara
David Vacca
Andrei Shikov
Lulu Wu
Kevin Gozali
Alessandro Sisto