Commit 72412fd
authored
fix: avoid reallocating views on RCTDevLoadingView showMessage calls (#2762)
This PR is equivalent to the upstream PR
facebook#54608. Actually, the
upstream PR is a _little_ different, as I had to handle the new
`self->_dismissButton` that was addeed in
facebook#54445 as well. When I came
to handle `self->_dismissButton`, I took the opportunity to optimise the
`NSConstraintLayout` bit while I was there, so be ready for both of
those changes in `[email protected]`!
## Summary:
As this PR regards the dev loading view, this issue of course affects
only **debug** builds. The issue is that, each time a Metro progress
update triggers the `showMessage:withColor:withBackgroundColor:` method
on `RCTDevLoadingView`, we end up allocating a new `UIWindow` (iOS) or
`NSWindow` (macOS), rather than reusing the existing window stored on
`self->_window` from any previous progress updates. These unnecessary
allocations are particularly expensive on macOS, as I show below.
### Demo of the issue on macOS
On `react-native-macos`, the impact of this issue is dramatic (so much
so that **the Microsoft Office team disable `RCTDevLoadingView`** in
their Mac app). On the first connection to Metro (as first-time
connections can produce nearly 100 progress updates), we end up
allocating nearly 100 `NSWindow`s. Allocating `NSWindow`s is so
expensive that it blocks the UI thread and **adds 30 seconds to app
launch** (see 00:40 -> 01:15 of the below video clip).
What's more, as we can see in the view debugger, these `NSWindow`s
**never get released**, so they remain in the view graph and just leak
memory (see 01:15 -> 01:30 of the below video clip).
(This clip is from 1:15:00 of a
[livestream](https://www.youtube.com/live/amRWVbfbknM?si=KmDBXjrQXdxnmf1E&t=4500)
where I dug into this problem – sorry for the poor quality clips, that's
the best the servers allow me to download)
https://github.com/user-attachments/assets/65bb7c9b-dc18-4e54-8369-d0c611c59439
## Solution
Each of these views (the window, the container, and the label) need only
be allocated once. Thereafter, they can be modified. This PR adds
conditionals to lazily-allocate them, and reorders them in order of
dependency (e.g. the label is the most nested, so needs to be handled
first).
## Changelog:
[MACOS] [FIXED] - Avoid reallocating views on RCTDevLoadingView progress
updates
## Test Plan:
I am unable to run `RNTester` on the latest commit of the `main` branch;
I've tried [five different
versions](https://x.com/birch_js/status/1991129150728642679?s=20) of
Ruby, but the `bundle install` always fails. If someone could please
guide me how to get RNTester, Ruby, and Bundler happy, I'd be happy to
use that app to test it on `main`.
So my testing so far has been based on patching an existing
`[email protected]` app live on stream. This version of React
Native macOS has the exact same bug as current `main`, so it should be
representative.1 parent b01a6d0 commit 72412fd
File tree
1 file changed
+27
-14
lines changed- packages/react-native/React/CoreModules
1 file changed
+27
-14
lines changedLines changed: 27 additions & 14 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
128 | 128 | | |
129 | 129 | | |
130 | 130 | | |
131 | | - | |
132 | | - | |
133 | | - | |
134 | | - | |
135 | | - | |
136 | | - | |
137 | | - | |
138 | 131 | | |
139 | | - | |
| 132 | + | |
140 | 133 | | |
141 | 134 | | |
142 | 135 | | |
143 | | - | |
| 136 | + | |
144 | 137 | | |
145 | 138 | | |
146 | 139 | | |
147 | 140 | | |
148 | 141 | | |
149 | 142 | | |
150 | | - | |
151 | 143 | | |
152 | | - | |
153 | | - | |
154 | | - | |
155 | 144 | | |
156 | 145 | | |
157 | | - | |
158 | 146 | | |
159 | 147 | | |
160 | 148 | | |
| |||
175 | 163 | | |
176 | 164 | | |
177 | 165 | | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
178 | 191 | | |
179 | 192 | | |
180 | 193 | | |
| |||
0 commit comments