Summary:
Calculation of TransformedFrames was a method introduced by D37994809 (https://github.com/facebook/react-native/commit/64528e5faa445907b8287b412c344f30c20fca61) to fix rendering of inverted flat lists.
We found that this operation is crashing in fb4a causing the UBN T127619309
This diff creates a feature flag to disable the calculation of TransformedFrames in Layoutable ShadowNodes. **The goal of this diff is to revert the behavior introduced by D37994809 (https://github.com/facebook/react-native/commit/64528e5faa445907b8287b412c344f30c20fca61)**
The featureFlag is disabled in fb4a and enabled in react AR (because ReactAr apps relies on the calculation of TransformedFrames and these apps are not affected)
The root cause of the bug will be fixed by D38280674 (which still requires more testing and investigation)
changelog: [internal] internal
Reviewed By: JoshuaGross
Differential Revision: D38286857
fbshipit-source-id: 721cd0554ae6a6b369b3f8dbb584160a270d0f18
Summary:
Fixes https://github.com/facebook/react-native/issues/34120
The new React Native architecture doesn't check `needsCustomLayoutForChildren` so it wrongly positions native views on Android. In https://github.com/facebook/react-native/issues/34120 there are videos comparing the positioning of a native action view in the old and the new architecture.
This PR passes the parent tag to the `updateLayout` method of the `SurfaceMountingManager`. The `SurfaceMountingManager` calls `needsCustomLayoutForChildren` on the parent view manager (copied the code from the `NativeViewHierarchyManager` in the old architecture).
**NOTE** - I wasn't sure where to get the parent shadow view from so I've put in my best guesses where I could and left it as `{}` otherwise.
## Changelog
[Android] [Fixed] - Migrate `needsCustomLayoutForChildren` check to the new architecture
Pull Request resolved: https://github.com/facebook/react-native/pull/34254
Test Plan:
I checked the fix in the repro from https://github.com/facebook/react-native/issues/34165. Here is a video of the action view closing using the native button that is now visible in the new architecture.
https://user-images.githubusercontent.com/1761227/180607896-35bf477f-4552-4b8a-8e09-9e8c49122c0c.mov
Reviewed By: cipolleschi
Differential Revision: D38153924
Pulled By: javache
fbshipit-source-id: e2c77fa70d725a33ce73fe4a615f6d884312580c
Summary: Changelog: [Internal] - We can now remove the '2' suffix as we had an internal implementation that was not truly aligned with W3C pointers but used the same name. We have aligned the internal types to match w3c so we can now remove the suffix that differentiates them.
Reviewed By: vincentriemer
Differential Revision: D37545813
fbshipit-source-id: 6f2336ae9e314066c340161113268c1f28621a71
Summary:
Internal tests are indicating that there's an OOM that happens during the RemoveDeleteTree worker loop while trying to expand the size of the tag stack.
In practice during manual testing I was not able to get the capacity beyond 40, but for deletions of very large trees, it's easy to imagine the size of the stack growing to at least the number of nodes in the tree being deleted. To mitigate this, we relax our requirements around the order in which onViewStateDeleted can be called and call it top-down instead of bottom up to maintain a smaller stack (see comments for more details).
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D37619259
fbshipit-source-id: 8f7af0137a868606a72fdc7bdca13c5e89b69573
Summary:
There is a very small volume of production errors caused by a View that is already in the hierarchy being added to the hierarchy again; this results in a crash on the Android platform layer.
We detect and attempt to silently recover from this case, while logging and collecting more diagnostics. This will still crash in debug mode.
It is unclear what layer this error is coming from: it could be an issue with the C++ differ (ordering, or something more tricky); that is unlikely but there are few other hypotheses at the moment.
Changelog: [Internal]
Reviewed By: javache
Differential Revision: D37557663
fbshipit-source-id: ffe320ff646e314fa921a2c2cf8058254713505c
Summary:
The initial version of this would result in LayoutAnimations running potentially much faster than 60FPS (incorrectly). Resolve by calling tryDispatchMountItems directly instead of the frame callback runner.
Changelog: [Internal]
Reviewed By: javache
Differential Revision: D37543693
fbshipit-source-id: 91dbd961ecc155221c84148cb6b252a4aac9ec91
Summary:
Crashes can occur if we try to disassemble trees not managed by React Native - for example a native component tree, Litho hierarchies, etc.
As we disassemble the tree, ensure that children are managed before disassembling a subtree.
Changelog: [Internal]
Reviewed By: ryancat
Differential Revision: D37493854
fbshipit-source-id: fee4d303133edcef663abfe8637bce6b24627724
Summary:
Due to the way we're dispatching queued MountItems on Android, we could be doing nothing on the UI thread for up to 15.99...ms before the start of the next frame, resulting in a delayed paint of up to 1 full (16ms) frame.
The delay is totally random and depends only on when the work is scheduled.
The tl;dr is that currently all MountItems are dispatched starting only at the /beginning/ of a UI frame. If we schedule work at FrameStart+0.000001ms, it will not be operated on until the start of the next frame, 16ms later. So the "wasted" time could be anywhere from 0 to ~16ms, but will result in at least 1 wasted frame regardless.
The fix is fairly trivial: start working on large buffered work as soon as we schedule it.
Changelog: [Android][Changed] Optimization to paint changes up to 1 frame earlier on Android/Fabric
Reviewed By: NickGerleman
Differential Revision: D37478885
fbshipit-source-id: 8af846736caf3a9d9f0d0c5e33328bebb87b1b32
Summary:
Instead of directly scheduling a Runnable on the UI thread, use a GuardedFrameCallback which (1) guards against exceptions thrown on the UI thread (in this case, errors in deferred remove/delete work really should not disrupt the UI at all or cause user-visible crashes) (2) allows us to split work across multiple frames if necessary (3) is more consistent with how we schedule other work on Android.
The only functionality change is that we might split work across multiple callbacks, in the case of tearing down a particularly large tree.
Changelog: [Internal]
Reviewed By: javache
Differential Revision: D37470531
fbshipit-source-id: d9d1fc85c29e53addea886db975c0d914581e618
Summary:
TL;DR: For applications using JS navigation, save 50-95% of CPU during mounting phase in N>2 navigations that replace ~most of screen.
During investigation of performance on the UI thread of React Native applications, I noticed that the /initial/ render of an screen for an application using JS navigation is /mostly/ consumed (on the UI thread) by tearing-down the previous View hierarchy. In one 185ms segment on the UI thread in production, 95% of the CPU time was Remove/Delete instructions and only 5% of CPU time was consumed by actually displaying the new hierarchy (this is specific to Android and also assumes that View Preallocation is being used, so post-commit work consists of Insert and UpdateLayout mutations primarily).
There are /some/ cases where the C++ differ knows that we are deleting an entire subtree and therefore we could communicate this to the mounting layer. All that matters is that these Views are removed from the View hierarchy immediately; and secondarily that their memory is cleaned up ASAP, but that doesn't need to happen immediately.
Some additional constraints and notes:
1) As noted in the comments, we cannot simply stop producing Remove and Delete instructions. We need to produce /both/ the new RemoveDeleteTree instruction, /and/ produce all the Remove/Delete instructions, primarily because LayoutAnimations relies heavily on these Remove/Delete instructions and certain things would break if we removed those instructions entirely. However, we can mark those Remove/Delete instructions as redundant, process them only in LayoutAnimations, and not send them to the Android mounting layer.
2) We want to make sure that View Recycling is not impacted. Since Android cannot take advantage of View Recycling until /after/ the second major render (preallocation of views will happen before any views are recycled), this doesn't impact View Recycling and we'll make sure Views are recycled whenever they are deleted.
Thus, we do two things:
1) Introduce a new RemoveDeleteTree operation that can delete an entire subtree recursively as part of one operation. This allows us to avoid serializing hundreds or thousands of instructions and prevents JNI traffic.
2) Besides removing the topmost View from the View hierarchy, and ensuring it's not drawn, the full teardown and recycling of the tree can happen /after/ the paint.
In some flows with JS navigation this saves us 95% of CPU during the mount phase. In the general case it is probably closer to 25-50% of CPU time that is saved and/or deferred.
Changelog: [Android][Changed] Significant perf optimization to Fabric Remove/Delete operations
Reviewed By: ryancat
Differential Revision: D37257864
fbshipit-source-id: a7d33fc74683939965cfb98be4db7890644110b2
Summary:
We currently wrap colors in an object to make it look similar to a `PlatformColor` object, but since this is a hot codepath, let's just optimize it to a simple array of strings. The next step is to apply a layer of caching here, but this should be a simple improvement.
Changelog: [internal]
Reviewed By: JoshuaGross
Differential Revision: D31057046
fbshipit-source-id: f68e17167ddd5bba3b545d039600c7c9b40808f5
Summary:
This diff adds an assertion to make sure the pending events are enqueued only when the event emitter is null. This is to avoid unexpected workflow when we queue events but we should just dispatch them.
Changelog:
[Android][Internal] - Make sure we only queue events when event emitter is null
Reviewed By: javache
Differential Revision: D36916482
fbshipit-source-id: fff305615b302ece26bc2482c826b74de4f70266
Summary:
This diff address an edge case when the pending events are enqueued when the surface is stopped. In this case we will reset map that holds view state to null, which will cause NPE.
Changelog:
[Android][Fixed] - Fix edge case when we enqueue a pending event to views on stopped surface
Reviewed By: javache, gorodscy
Differential Revision: D36912786
fbshipit-source-id: 3ae5a4b08a0a6bf55538d69ac80a101c2c3d899a
Summary:
Followups to View Recycling diffs to improve things / clean up things a bit. This also fixes memory warnings which were not hooked up before.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D36707792
fbshipit-source-id: 410e70bf0eeec5569566138af547e1601394d0e6
Summary:
This diff cleans up several Android Makefiles which we're not using anymore
as they've been replaced by CMake files.
There are still 3 Makefiles left, which I'm aiming to remove in the near future.
Changelog:
[Internal] [Changed] - Remove unused Makefiles from React Native core
Reviewed By: javache
Differential Revision: D36660902
fbshipit-source-id: 8afffac74d493616b0f9414567821cd69f4ef803
Summary:
There's no need for this to be a setter/getter, as there are no side-effects, and it means we can use the same helper method to read it as other feature flags.
Changelog: [Internal]
Reviewed By: cortinico
Differential Revision: D36595602
fbshipit-source-id: d27c01bd39d03606a6f8c17ba181a8cd0bf58dbb
Summary:
It's useful to have more systrace markers that are all the same, so that they can all be aggregated and work underneath them aggregated across an entire trace. As it is, this marker gets treated as unique nearly every time which makes analysis harder.
Changelog: [Internal]
Reviewed By: javache
Differential Revision: D36533075
fbshipit-source-id: 925afa7db152eca1166891b41e7c6f6a511840af
Summary: Changelog: [Internal] - Bypass dispatching an event if no view along the hierarchy is listening to it. Only applied for touch-based interactions. Next change will add optimization for mouse interactions
Reviewed By: vincentriemer
Differential Revision: D35739417
fbshipit-source-id: 134ffefef3bb4f97bf3e63b6bccc0caca464dfbd
Summary:
Splitting these changes of from D36482630, which is a minor improvement to the singleOp experiment. Since we call start/finish already on the native side, we don't need to repeat it from JS.
Changelog: [Internal]
Reviewed By: JoshuaGross
Differential Revision: D36541700
fbshipit-source-id: 7d61669710faca3153be557fb2d214011eda87c5
Summary:
`test_android` is currently broken as it tries to build with `source = "7"` (the default).
This is a best guess fix to try to fix this issue.
## Changelog
[Internal] - Attempt to fix test_android by specifying source 8
Pull Request resolved: https://github.com/facebook/react-native/pull/33890
Test Plan: Will rely on CI
Reviewed By: cipolleschi
Differential Revision: D36589814
Pulled By: cortinico
fbshipit-source-id: 8ef50f8531b9d4367d2f2c75a312e8fdaf38fd85
Summary:
I'm finding it useful to keep track of a few additional numbers when doing perf analysis in Fabric.
Also making it easier to enable just these perf logs without all of the other verbose Fabric logs.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D36500518
fbshipit-source-id: e57e1e75131c5d59da876d1decb96d4da386f025
Summary:
Noticed that we emit a large amount of (admittedly cheap) mountitems as part of node creation for values that are all zero (e.g. padding, overflowinset), which we can assume to be already initialised with these values on the native side.
There's a further opportunity to do this for State as well, as ReactImageComponentState exports just empty maps to Java.
Changelog: [Internal]
Reviewed By: genkikondo
Differential Revision: D36345402
fbshipit-source-id: 8d776ca124bdb9e1cd4de57a04e2785a9a0f918c
Summary:
This diff fixed an edge case that event dispatching is failed after pre-allocation of a view and before the view is mounted.
When a cached image is loaded, we will dispatch the event to JS immediately. This is could happen after the view is created during pre-allocation phase, when the event emitter is not instantiated yet. In that case, we will see [an error](https://github.com/facebook/react-native/blob/main/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java#L927) and the event will effectively be ignored.
To fix that we introduced a queue in this diff for those events. They will be dispatched in order when the view is mounted and the event emitter is non-null.
Changelog:
[Android][Fixed] - Fixed an edge case that event dispatching is failed after pre-allocation of a view and before the view is mounted.
Reviewed By: mullender
Differential Revision: D36331914
fbshipit-source-id: cd065b0b36978cb5f0aac793d8d16f07a48f0881
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/33776
The implementations of these modules is pretty much identical, and we're already shipping the fbjni version of this anyway.
Changelog: [Internal]
Reviewed By: mhorowitz
Differential Revision: D36200330
fbshipit-source-id: 135ee621e1e4c5eb9616ce7f442fc6d4b946f865
Summary:
Cherry picking https://github.com/facebook/react-native/pull/33707 to main branch
This change is extending the changes made by alespergl to reduce the file paths and command lengths of ndk build commands
Essentially we are shortening the length of the source files by using relative paths instead of absolute paths as enumerated by the wildcard expression
This commit is extending the fix by including all the new modules introduced into RN for the new architecture, including the generated modules.
We are also reverting the ndk bump as ndk23 is crashing frequently when building RN with new arch. The reduced file paths lengths ensures the ndk bump is not required for relatively short application paths.
Fix building RN with new architecture on Windows boxes by using relative paths for C++ sources
## Changelog
Fix building RN with new architecture on Windows boxes by using relative paths for C++ sources
[CATEGORY] [TYPE] - Message
Pull Request resolved: https://github.com/facebook/react-native/pull/33784
Test Plan: Verified building on windows box
Reviewed By: javache
Differential Revision: D36241928
Pulled By: cortinico
fbshipit-source-id: 1ce428a271724cbd3b00a24fe03e7d69253f169b
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/33780
Currently this just has a default ThreadPool name, which can be confusing in systraces and crash reports.
Changelog: [Internal]
Reviewed By: ryancat
Differential Revision: D36200090
fbshipit-source-id: 22918993e7c822ed721ccaf79cdcd9d2a972193d
Summary: There's no need to use private feature flag for overflowInset feature. This makes future refactor on feature flags easier.
Reviewed By: javache
Differential Revision: D35866302
fbshipit-source-id: a3fde212ff40cd6d4c68832bb39b34de5a17a8d4
Summary:
This diff fixes a NullPointerException when disaptching events while the SurfaceMountingManager is being destroyed
changelog: [android] android
Reviewed By: cortinico
Differential Revision: D35559550
fbshipit-source-id: c07f74493384fb1b306338ec1bc8b96f1b6f1f41
Summary:
Now that the PFH node has been renamed this updates the pfh label.
Produced via `xbgs -l -e '"pfh:ReactNative_CommonInfrastructurePlaceholde"' | xargs sed -i 's/"pfh:ReactNative_CommonInfrastructurePlaceholde"/"pfh:ReactNative_CommonInfrastructurePlaceholder"/'`
Reviewed By: jkeljo
Differential Revision: D35374087
fbshipit-source-id: 61590f69de5a69ec3b8a0478f6dd43409de3c70b
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/33535
Hack to support Kotlin functions in Buck compilation: adds Kotlin stdlib as a dependency to make sure upstream targets include Kotlin jvm internal classes.
Changelog: [Internal]
Reviewed By: cortinico
Differential Revision: D35284430
fbshipit-source-id: 0d29ad30386514c8df5376d0a6809d3105f0cd0f
Summary:
Aligns naming with `JWritableMapBuffer` and other fbjni classes to indicate that it is a JNI binding.
Changelog: [Internal] - Rename C++ part of ReadableMapBuffer to JReadableMapBuffer
Reviewed By: mdvacca
Differential Revision: D35219323
fbshipit-source-id: a7eb644a700a35dc94fa18e4fb3cc68f2cfa3e99
Summary:
Updates `ReadableMapBuffer` to conform to `MapBuffer` interface, to allow interchangeable use of `Readable/WritableMapBuffer` in the code.
Notable changes:
- MapBuffer.Entry getters are now represented as Kotlin properties and appended `Value` suffix (e.g. `entry.getInt()` becomes `entry.getIntValue()` in Java, or `entry.intValue` in Kotlin)
- `ByteBuffer` is imported in constructor instead of on demand. This method doesn't copy the data as the bytes are read directly from native heap, and benchmarking a 500-byte `MapBuffer` shows no difference in import speed.
- Internal logic of `ReadableMapBuffer` uses `UShort` kotlin type for key retrieval, for more correct representation of values.
- Explicit exception throws are replaced with `require` and `check` methods for `IllegalArgumentException` and `IllegalStateException` (default FB conversion).
The change also updates `ReadableMapBuffer` usages to `MapBuffer` interface where possible (virtually everywhere except JNI methods).
Changelog: [Android][Changed] - Adopt `MapBuffer` interface for `ReadableMapBuffer`
Reviewed By: mdvacca
Differential Revision: D35218633
fbshipit-source-id: 515dd974c27b2978ade325b2e1750ab8f068a20a
Summary:
While it would be better to be able to do all of the ownership metadata at the Buck macro level, that proved to be more work than expected.
This diff adds the corresponding pfh label to all targets in `xplat/js/react-native-github` that have a Supermodule label. Once the migration is complete the Supermodules labels will be able to be removed.
Reviewed By: cortinico
Differential Revision: D35221544
fbshipit-source-id: d87d5e266dfb5e6ee087251dc34dff5db299bbaf
Summary:
Aligns two codepaths for measure, making sure we can use both MapBuffer and ReadableMap for measuring components.
Changelog: [Internal] - Align measure interface for MapBuffer experiment
Reviewed By: javache, mdvacca
Differential Revision: D34960317
fbshipit-source-id: a39eb84a0abb4414717463f2f1741e470be3531f
David Vacca
Graham Mendick
Luna Wei
Kevin Gozali
Joshua Gross
Lulu Wu
Pieter De Baets
Xin Chen
Nicola Corti
Rubén Norte
Anandraj Govindan
Chris Olszewski
Andrei Shikov
CodemodService Bot