Summary:
sSpannableCache is a significant user of Java heap memory - up to 0.22MB is retained by sSpannableCache.
It turns out sSpannableCache was never hitting as hashCode is different for the same attributedString contents. attributedString.getInt("hash") provides the expected hash code.
This indicates removing spannableCache will not affect perf. Will gate just in case though.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D34900414
fbshipit-source-id: 7563cde6ba9dc153072e7aebede99389ce3153e7
Summary:
Blocks on queue write/drain for Animated module under a feature flag to test whether it resolves race conditions.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D34752947
fbshipit-source-id: a1b1a286772d29a7a27b5e9c3f743cac84cc2bab
Summary:
changelog: [internal]
ViewState is sometimes deleted but not created again because of race condition in C++ pre-allocation and how it checks for revision to determine if createView mount item should be dispatched.
In this diff, we add an option to delay deletion of ViewState until the next commit.
Reviewed By: ShikaSD
Differential Revision: D34553174
fbshipit-source-id: e5213f1d39137fb56fc745321a7c1b28d417ea27
Summary:
Changelog: [Internal]
Remove all the MCs that enable SVCs in Fabric, because we'll only test SVCs in Bridgeless mode to simplify rollout. There were complications with enabling SVCs in Fabric at a previous rollout.
Reviewed By: RSNara
Differential Revision: D33861243
fbshipit-source-id: fdbfedce77f8bd1bab2a807237017787ae8bf7c1
Summary:
This diff deletes the flag ReactFeatureFlags.enableReactContextCleanupFix, the flag was disabled for many months, I just disable it.
changelog: [internal] internal
Reviewed By: genkikondo, makovkastar
Differential Revision: D33781628
fbshipit-source-id: 4b5e22adf9d30da5b85bbbde8bdc98d98f5e8891
Summary:
This diff deletes the ReactFeatureFlag enableFabricInLogBox, from now on Logbox will be rendered in Fabric when Fabric is enabled
changelog: [internal] internal
Reviewed By: genkikondo
Differential Revision: D33781626
fbshipit-source-id: 3187a22fec80125afd27860995637564640dab8d
Summary:
Enable eager intialization of FabricUIManager during intiialization of React Native.
This feature highly improved TTRC in Markeptlace Home
changelog: [internal] internal
Reviewed By: genkikondo
Differential Revision: D33585099
fbshipit-source-id: 0ffbc720bcb1edd1b04180189a52c82e9e2fa800
Summary:
The previous diff shows how `overflowInset` could improve hit test algorithm performance. This diff adds experiment to it in order to:
1. Understand the perf improvement in production
2. Provide quick way to rollback in production -- the changes are used by FB4A as well as VR
3. The changes will pass more instructions over JNI, which may have impact on perf.
To share the MC param values in both Java and C++ side, the value is hosted by `ReactFeatureFlags` and fetched in `FbReactInstanceHolder`.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D33179310
fbshipit-source-id: 327100d41f7b5a668ff0d2afabcdd1fc16cb5a18
Summary:
With the updates to touch processing rolled out, we can remove the feature flag and clean up the old code. The old path is now used exclusively by legacy renderer and Fabric uses new `EventEmitter#receiveTouches` method to process touches.
Changelog:
[Changed][Android] - Update touch processing internals
Reviewed By: mdvacca
Differential Revision: D32953664
fbshipit-source-id: 517a4ce6ce9bc15528c2db94d7d11bdff8b78743
Summary:
Originally introduced in D2022018
Tried to make the processor optional when no rounding is required, but found even that was not strictly necessary.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D32675492
fbshipit-source-id: 8dfdbf0e4347155045f77b1fba00a59086fe7a33
Summary:
Changelog: [Internal]
# Context
Whilst looking at Marketplace Loom traces, ShikaSD made the remark that we could potentially start the JS work much earlier instead of waiting for the View.onMeasure call, a behaviour that is already built-in Venice.
Reviewed By: ShikaSD
Differential Revision: D32559505
fbshipit-source-id: cc6337955ad2b6a6581a0347f1f976679eaca54d
Summary:
This bug is caused by RNAndroid dispatching an incorrect sequence of events to JS when the user taps on a Text.
Taking into consideration the example P462662009, when the user taps of the "Inner" text, RN Android is dispatching three events:
topTouchStart, topTouchStart and topTouchEnd.
The information stored on the first two JS events is correct, but the problem is that it is duplicated. This sequence of events makes Pressability to dispatch the event to the incorrect target.
This was originally introduced in D3035589 (https://github.com/facebook/react-native/commit/39fdce259dd46de8fd715efbfd12ede4bc24c8c2) (2016)
In this diff I'm changing the way RN Android bubbles events when the user taps on a ReactTextView. From now on, events won't be bubbled anymore, and they will be handled by the ReactRootView.onInterceptTouchEvent: https://fburl.com/code/rbt8$
Additionally, I'm creating a FeatureFlag in case this change has a unknown side effect that's only detected in production.
I will create a MC for FB4A in the next diffs of the stack
changelog: [Fixed][Android] Fix onPress event for nested Text in RN Android
Reviewed By: javache
Differential Revision: D31628461
fbshipit-source-id: 177397d4369191a3c97e2f86e801757b27ee5121
Summary:
Updates touch events in Fabric to be dispatched through the same pipeline as the rest of events, instead of relying on custom dispatch behavior.
Previous method of handling touches was reusing Paper behavior which required:
1. Transform event into a Paper-compatible form of WritableArray and dispatch it to `RCTEventEmitter.receiveTouches`.
2. Intercept `receiveTouches` for Fabric and redirect it to `FabricEventEmitter`
3. Perform transformations copied from Paper JS renderer in Java, transform it to the final form and dispatch this event as usual after.
The new behavior uses emitter's `receiveEvent` method directly to dispatch events. Additionally, it should decrease allocations done when transforming events during step 3 above, as `WritableNativeMap`-based operations performed many re-allocations when reading/re-creating arrays.
Changelog:
[Android][Changed] - Added an experimental touch dispatch path
Reviewed By: JoshuaGross
Differential Revision: D31280052
fbshipit-source-id: 829c2646ac6b0ebff0f0106159e76d84324ac732
Summary:
Event merging or "coalescing" is done on Java side from Android, but Fabric also includes some Cxx logic to merge those events. Although Android doesn't need this logic in particular, it is important to follow this path to ensure these events (e.g. scroll) are dispatched as "continuous", allowing for correct prioritization in Concurrent Mode.
`dispatchModernV2` selects between `dispatch` and `dispatchUnique` based on the `canBeCoalesced` parameter of the event, which is exactly what we need. The logic is only used in the "new" event dispatcher at the moment, so I wrapped it with feature flag to validate it doesn't cause any regressions.
Changelog:
[Android][Internal] - Try dispatching coalescing events as unique to Fabric
Reviewed By: sammy-SC
Differential Revision: D31272585
fbshipit-source-id: 6b67b61bd13fbff019d9eb8c5172bdd814a7b5b8
Summary:
Uses `enableZ` trick to ensure that Skia doesn't try to reorder the views based on elevation. Unfortunately, it only helps for some cases, but the proper fix would require reimplementing `ViewGroup` completely to remove its internal logic of reordering based on Z coordinate.
Changelog: [Android][Fixed] Ensure elevated views are behind sticky header in FlatList
Reviewed By: cortinico
Differential Revision: D30700566
fbshipit-source-id: d2c59b22332922c610f4f2d415df34e81f5a33c5
Summary:
When true, this flag will cause React Native to start logging soft exceptions, whenever the legacy NativeModule system is used. This flag is independent of useTurboModules. In practice, it's only enabled when TurboModules is enabled.
Changelog: [Internal]
Reviewed By: JoshuaGross
Differential Revision: D30250363
fbshipit-source-id: f610567c5b99a4fbf8252c3962908668f483d028
Summary:
This is the Android analogue to D30019833.
Changelog: [Internal]
Reviewed By: p-sun
Differential Revision: D30029295
fbshipit-source-id: 13df0dfb915697eeedcc527dcdb6c246e89afb0c
Summary:
As a followup to T91209139, ship "state update scroll race" in code. This also ships it for HorizontalScrollView since it's been validated for vertical scroll views.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D29595601
fbshipit-source-id: 64b6a23e2dab2c13123e132d9d899fb769d03172
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
