fix(ios): avoid reallocating views on RCTDevLoadingView showMessage calls#54608
Closed
shirakaba wants to merge 6 commits into
Closed
fix(ios): avoid reallocating views on RCTDevLoadingView showMessage calls#54608shirakaba wants to merge 6 commits into
shirakaba wants to merge 6 commits into
Conversation
Contributor
|
Thanks for this! I've tested on RNTester internally and things seem to work fine. |
…dow-in-rctdevloadingview # Conflicts: # packages/react-native/React/CoreModules/RCTDevLoadingView.mm
97e28f4 to
422f184
Compare
Contributor
Author
|
I've updated the PR to catch up with the recent changes in #54445 that introduced a new dismiss button. Again, I avoid allocations (so this PR fixes the potential allocation of a new I tested it as follows: # Initialise an app using the latest nightly build:
npx @react-native-community/cli init pr54608 --version 0.84.0-nightly-20251121-2bbb104a4
# ... Then patch `node_modules/react-native/React/CoreModules/RCTDevLoadingView.mm` with my PR code.Here's a visual regression test:
… No regressions! |
…dow-in-rctdevloadingview
Collaborator
|
This pull request was successfully merged by @shirakaba in 55a5b6b When will my fix make it into a release? | How to file a pick request? |
Saadnajmi
pushed a commit
to microsoft/react-native-macos
that referenced
this pull request
Dec 2, 2025
…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 `react-native-macos@0.84.0`! ## 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 `react-native-macos@0.79.0` app live on stream. This version of React Native macOS has the exact same bug as current `main`, so it should be representative.
gabrieldonadel
pushed a commit
to gabrieldonadel/not-react-native-macos
that referenced
this pull request
Dec 30, 2025
…(#2762) This PR is equivalent to the upstream PR facebook/react-native#54608. Actually, the upstream PR is a _little_ different, as I had to handle the new `self->_dismissButton` that was addeed in facebook/react-native#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 `react-native-macos@0.84.0`! ## 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 `react-native-macos@0.79.0` app live on stream. This version of React Native macOS has the exact same bug as current `main`, so it should be representative.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.




(See also the equivalent PR for
react-native-macos@0.79.x: microsoft#2762)Summary:
I'm upstreaming a change I've made to
RCTDevLoadingViewfor the sake ofreact-native-macos, as I believereact-nativecould benefit from the same change.As it 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 onRCTDevLoadingView, we end up allocating a newUIWindow(iOS) orNSWindow(macOS), rather than reusing the existing window stored onself->_windowfrom 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 disableRCTDevLoadingViewin their Mac app). On the first connection to Metro (as first-time connections can produce nearly 100 progress updates), we end up allocating nearly 100NSWindows. AllocatingNSWindows 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
NSWindows 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 where I dug into this problem – sorry for the poor quality clips, that's the best the servers allow me to download)
mwe_clip.mp4
Demo of the issue on iOS
The problem is, fortunately, unnoticeable on iOS (which is perhaps why it was overlooked when ported to macOS). I ran the same test on
react-native@0.79.7and found that although React Native iOS does needlessly allocate manyUIWindows, the allocations don't seem to delay app launch and no dangling windows show up in the view debugger. So it's possible that it's cheaper to allocate and dispose of windows in UIKit than in AppKit.(This clip is from 3:03:45 of the livestream:)
output.mp4
But still, I feel it's good to align implementations and just good practice to avoid allocating memory needlessly, so I'd like to open this PR nonetheless. Heck, it might still be important for iOS on larger codebases that emit many more progress updates than the Hello World app demoed here.
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:
[IOS] [FIXED] - Avoid reallocating views on RCTDevLoadingView progress updates
Test Plan:
I am unable to run
RNTesteron the latest commit of themainbranch; I've tried five different versions of Ruby, but thebundle installalways 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 onmain.So my testing so far has been based on patching an existing
react-native@0.79.7app live on stream. This version of React Native has the exact same bug as currentmain, so it should be representative.