Delete ShadowTreeRevision on background thread #50997
Closed
Conversation
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
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D73688009 |
rozele
added a commit
to rozele/react-native-macos
that referenced
this pull request
Apr 29, 2025
Summary: In some apps, we spend a non-trivial amount of time calling ShadowNode destructors on the UI thread. A simple way to avoid stalling the UI thread is to move the `baseRevision_` instance to a data structure that is cleared on a background thread, so it's tree of ShadowNode shared_ptrs are released (and in most cases destroyed) on the background thread. To minimize thread initialization costs, this change adds an AsyncDestructor helper that uses a single background thread and a queue of to-be-destroyed objects that are released in a run loop. This change is also guarded by a feature flag so we can keep an eye out for potential memory leaks. ## Changelog [Internal] Differential Revision: D73688009
rozele
force-pushed
the
export-D73688009
branch
from
c793405 to
4770bcd
Compare
|
This pull request was exported from Phabricator. Differential Revision: D73688009 |
rozele
force-pushed
the
export-D73688009
branch
from
4770bcd to
4352c62
Compare
rozele
added a commit
to rozele/react-native-macos
that referenced
this pull request
Apr 29, 2025
Summary: In some apps, we spend a non-trivial amount of time calling ShadowNode destructors on the UI thread. A simple way to avoid stalling the UI thread is to move the `baseRevision_` instance to a data structure that is cleared on a background thread, so it's tree of ShadowNode shared_ptrs are released (and in most cases destroyed) on the background thread. To minimize thread initialization costs, this change adds an AsyncDestructor helper that uses a single background thread and a queue of to-be-destroyed objects that are released in a run loop. This change is also guarded by a feature flag so we can keep an eye out for potential memory leaks. ## Changelog [Internal] Differential Revision: D73688009
|
This pull request was exported from Phabricator. Differential Revision: D73688009 |
rozele
added a commit
to rozele/react-native-macos
that referenced
this pull request
Apr 29, 2025
Summary: Pull Request resolved: facebook#50997 In some apps, we spend a non-trivial amount of time calling ShadowNode destructors on the UI thread. A simple way to avoid stalling the UI thread is to move the `baseRevision_` instance to a data structure that is cleared on a background thread, so it's tree of ShadowNode shared_ptrs are released (and in most cases destroyed) on the background thread. To minimize thread initialization costs, this change adds an AsyncDestructor helper that uses a single background thread and a queue of to-be-destroyed objects that are released in a run loop. This change is also guarded by a feature flag so we can keep an eye out for potential memory leaks. ## Changelog [Internal] Differential Revision: D73688009
rozele
force-pushed
the
export-D73688009
branch
2 times, most recently
from
516dda4 to
76035b5
Compare
rozele
added a commit
to rozele/react-native-macos
that referenced
this pull request
Apr 29, 2025
Summary: In some apps, we spend a non-trivial amount of time calling ShadowNode destructors on the UI thread. A simple way to avoid stalling the UI thread is to move the `baseRevision_` instance to a data structure that is cleared on a background thread, so it's tree of ShadowNode shared_ptrs are released (and in most cases destroyed) on the background thread. To minimize thread initialization costs, this change adds an AsyncDestructor helper that uses a single background thread and a queue of to-be-destroyed objects that are released in a run loop. This change is also guarded by a feature flag so we can keep an eye out for potential memory leaks. ## Changelog [Internal] Differential Revision: D73688009
|
This pull request was exported from Phabricator. Differential Revision: D73688009 |
rozele
added a commit
to rozele/react-native-macos
that referenced
this pull request
Apr 29, 2025
Summary: Pull Request resolved: facebook#50997 In some apps, we spend a non-trivial amount of time calling ShadowNode destructors on the UI thread. A simple way to avoid stalling the UI thread is to move the `baseRevision_` instance to a data structure that is cleared on a background thread, so it's tree of ShadowNode shared_ptrs are released (and in most cases destroyed) on the background thread. To minimize thread initialization costs, this change adds an AsyncDestructor helper that uses a single background thread and a queue of to-be-destroyed objects that are released in a run loop. This change is also guarded by a feature flag so we can keep an eye out for potential memory leaks. ## Changelog [Internal] Differential Revision: D73688009
rozele
force-pushed
the
export-D73688009
branch
from
76035b5 to
3620cfd
Compare
rozele
added a commit
to rozele/react-native-macos
that referenced
this pull request
Apr 29, 2025
Summary: In some apps, we spend a non-trivial amount of time calling ShadowNode destructors on the UI thread. A simple way to avoid stalling the UI thread is to move the `baseRevision_` instance to a data structure that is cleared on a background thread, so it's tree of ShadowNode shared_ptrs are released (and in most cases destroyed) on the background thread. To minimize thread initialization costs, this change adds an AsyncDestructor helper that uses a single background thread and a queue of to-be-destroyed objects that are released in a run loop. This change is also guarded by a feature flag so we can keep an eye out for potential memory leaks. ## Changelog [Internal] Differential Revision: D73688009
rozele
force-pushed
the
export-D73688009
branch
from
3620cfd to
df7cbd9
Compare
|
This pull request was exported from Phabricator. Differential Revision: D73688009 |
rozele
force-pushed
the
export-D73688009
branch
from
df7cbd9 to
fa6b8c4
Compare
rozele
added a commit
to rozele/react-native-macos
that referenced
this pull request
Apr 30, 2025
Summary: In some apps, we spend a non-trivial amount of time calling ShadowNode destructors on the UI thread. A simple way to avoid stalling the UI thread is to move the `baseRevision_` instance to a data structure that is cleared on a background thread, so it's tree of ShadowNode shared_ptrs are released (and in most cases destroyed) on the background thread. Following how most other threading in react-native works, this change also introduces a thread pool abstraction that should be supplied by host platforms (LowPriorityThreadPool). The implementation of this LowPriorityThreadPool will be introduced in future diffs for each platform. The AsyncDestructor abstraction simply moves the object into a captured variable and dispatches it to the LowPriorityThreadPool. This change is also guarded by a feature flag so we can keep an eye out for potential memory leaks. ## Changelog [Internal] Differential Revision: D73688009
|
This pull request was exported from Phabricator. Differential Revision: D73688009 |
rozele
added a commit
to rozele/react-native-macos
that referenced
this pull request
Apr 30, 2025
Summary: In some apps, we spend a non-trivial amount of time calling ShadowNode destructors on the UI thread. A simple way to avoid stalling the UI thread is to move the `baseRevision_` instance to a data structure that is cleared on a background thread, so it's tree of ShadowNode shared_ptrs are released (and in most cases destroyed) on the background thread. Rather than using std::thread, this change introduces the LowPriorityExecutor abstraction that should be supplied by host platforms. The implementation of this LowPriorityExecutor will be introduced in future diffs for each platform. Moving the ShadowTreeRevision into a lambda capture and punting the lambda to the LowPriorityExecutor moves the destructor calls of the ShadowNodes to the host platform implementation of the LowPriorityExecutor. This change is also guarded by a feature flag so we can keep an eye out for potential memory leaks. ## Changelog [Internal] Differential Revision: D73688009
rozele
force-pushed
the
export-D73688009
branch
from
fa6b8c4 to
2160b50
Compare
|
This pull request was exported from Phabricator. Differential Revision: D73688009 |
rozele
added a commit
to rozele/react-native-macos
that referenced
this pull request
Apr 30, 2025
Summary: In some apps, we spend a non-trivial amount of time calling ShadowNode destructors on the UI thread. A simple way to avoid stalling the UI thread is to move the `baseRevision_` instance to a data structure that is cleared on a background thread, so it's tree of ShadowNode shared_ptrs are released (and in most cases destroyed) on the background thread. Rather than using std::thread, this change introduces the LowPriorityExecutor abstraction that should be supplied by host platforms. The implementation of this LowPriorityExecutor will be introduced in future diffs for each platform. Moving the ShadowTreeRevision into a lambda capture and punting the lambda to the LowPriorityExecutor moves the destructor calls of the ShadowNodes to the host platform implementation of the LowPriorityExecutor. This change is also guarded by a feature flag so we can keep an eye out for potential memory leaks. ## Changelog [Internal] Differential Revision: D73688009
rozele
force-pushed
the
export-D73688009
branch
from
2160b50 to
894c0b9
Compare
|
This pull request was exported from Phabricator. Differential Revision: D73688009 |
rozele
added a commit
to rozele/react-native-macos
that referenced
this pull request
May 2, 2025
Summary: Pull Request resolved: facebook#50997 In some apps, we spend a non-trivial amount of time calling ShadowNode destructors on the UI thread. A simple way to avoid stalling the UI thread is to move the `baseRevision_` instance to a data structure that is cleared on a background thread, so it's tree of ShadowNode shared_ptrs are released (and in most cases destroyed) on the background thread. Rather than using std::thread, this change introduces the LowPriorityExecutor abstraction that should be supplied by host platforms. The implementation of this LowPriorityExecutor for each platform is as follows: - iOS: uses dispatch_async to a low priority dispatch queue - Android: uses a pthread with SCHED_OTHER and priority = 19 Moving the ShadowTreeRevision into a lambda capture and punting the lambda to the LowPriorityExecutor moves the destructor calls of the ShadowNodes to the host platform implementation of the LowPriorityExecutor. This change is also guarded by a feature flag so we can keep an eye out for potential memory leaks. ## Changelog [Internal] Reviewed By: NickGerleman Differential Revision: D73688009
|
This pull request was exported from Phabricator. Differential Revision: D73688009 |
rozele
added a commit
to rozele/react-native-macos
that referenced
this pull request
May 2, 2025
Summary: Pull Request resolved: facebook#50997 In some apps, we spend a non-trivial amount of time calling ShadowNode destructors on the UI thread. A simple way to avoid stalling the UI thread is to move the `baseRevision_` instance to a data structure that is cleared on a background thread, so it's tree of ShadowNode shared_ptrs are released (and in most cases destroyed) on the background thread. Rather than using std::thread, this change introduces the LowPriorityExecutor abstraction that should be supplied by host platforms. The implementation of this LowPriorityExecutor for each platform is as follows: - iOS: uses dispatch_async to a low priority dispatch queue - Android: uses a pthread with SCHED_OTHER and priority = 19 Moving the ShadowTreeRevision into a lambda capture and punting the lambda to the LowPriorityExecutor moves the destructor calls of the ShadowNodes to the host platform implementation of the LowPriorityExecutor. This change is also guarded by a feature flag so we can keep an eye out for potential memory leaks. ## Changelog [Internal] Reviewed By: NickGerleman Differential Revision: D73688009
|
This pull request was exported from Phabricator. Differential Revision: D73688009 |
rozele
added a commit
to rozele/react-native-macos
that referenced
this pull request
May 2, 2025
Summary: Pull Request resolved: facebook#50997 In some apps, we spend a non-trivial amount of time calling ShadowNode destructors on the UI thread. A simple way to avoid stalling the UI thread is to move the `baseRevision_` instance to a data structure that is cleared on a background thread, so it's tree of ShadowNode shared_ptrs are released (and in most cases destroyed) on the background thread. Rather than using std::thread, this change introduces the LowPriorityExecutor abstraction that should be supplied by host platforms. The implementation of this LowPriorityExecutor for each platform is as follows: - iOS: uses dispatch_async to a low priority dispatch queue - Android: uses a pthread with SCHED_OTHER and priority = 19 Moving the ShadowTreeRevision into a lambda capture and punting the lambda to the LowPriorityExecutor moves the destructor calls of the ShadowNodes to the host platform implementation of the LowPriorityExecutor. This change is also guarded by a feature flag so we can keep an eye out for potential memory leaks. ## Changelog [Internal] Reviewed By: NickGerleman Differential Revision: D73688009
|
This pull request was exported from Phabricator. Differential Revision: D73688009 |
rozele
added a commit
to rozele/react-native-macos
that referenced
this pull request
May 2, 2025
Summary: Pull Request resolved: facebook#50997 In some apps, we spend a non-trivial amount of time calling ShadowNode destructors on the UI thread. A simple way to avoid stalling the UI thread is to move the `baseRevision_` instance to a data structure that is cleared on a background thread, so it's tree of ShadowNode shared_ptrs are released (and in most cases destroyed) on the background thread. Rather than using std::thread, this change introduces the LowPriorityExecutor abstraction that should be supplied by host platforms. The implementation of this LowPriorityExecutor for each platform is as follows: - iOS: uses dispatch_async to a low priority dispatch queue - Android: uses a pthread with SCHED_OTHER and priority = 19 Moving the ShadowTreeRevision into a lambda capture and punting the lambda to the LowPriorityExecutor moves the destructor calls of the ShadowNodes to the host platform implementation of the LowPriorityExecutor. This change is also guarded by a feature flag so we can keep an eye out for potential memory leaks. ## Changelog [Internal] Reviewed By: NickGerleman Differential Revision: D73688009
|
This pull request was exported from Phabricator. Differential Revision: D73688009 |
rozele
added a commit
to rozele/react-native-macos
that referenced
this pull request
May 2, 2025
Summary: Pull Request resolved: facebook#50997 In some apps, we spend a non-trivial amount of time calling ShadowNode destructors on the UI thread. A simple way to avoid stalling the UI thread is to move the `baseRevision_` instance to a data structure that is cleared on a background thread, so it's tree of ShadowNode shared_ptrs are released (and in most cases destroyed) on the background thread. Rather than using std::thread, this change introduces the LowPriorityExecutor abstraction that should be supplied by host platforms. The implementation of this LowPriorityExecutor for each platform is as follows: - iOS: uses dispatch_async to a low priority dispatch queue - Android: uses a pthread with SCHED_OTHER and priority = 19 Moving the ShadowTreeRevision into a lambda capture and punting the lambda to the LowPriorityExecutor moves the destructor calls of the ShadowNodes to the host platform implementation of the LowPriorityExecutor. This change is also guarded by a feature flag so we can keep an eye out for potential memory leaks. ## Changelog [Internal] Reviewed By: NickGerleman Differential Revision: D73688009
|
This pull request was exported from Phabricator. Differential Revision: D73688009 |
rozele
added a commit
to rozele/react-native-macos
that referenced
this pull request
May 2, 2025
Summary: Pull Request resolved: facebook#50997 In some apps, we spend a non-trivial amount of time calling ShadowNode destructors on the UI thread. A simple way to avoid stalling the UI thread is to move the `baseRevision_` instance to a data structure that is cleared on a background thread, so it's tree of ShadowNode shared_ptrs are released (and in most cases destroyed) on the background thread. Rather than using std::thread, this change introduces the LowPriorityExecutor abstraction that should be supplied by host platforms. The implementation of this LowPriorityExecutor for each platform is as follows: - iOS: uses dispatch_async to a low priority dispatch queue - Android: uses a pthread with SCHED_OTHER and priority = 19 Moving the ShadowTreeRevision into a lambda capture and punting the lambda to the LowPriorityExecutor moves the destructor calls of the ShadowNodes to the host platform implementation of the LowPriorityExecutor. This change is also guarded by a feature flag so we can keep an eye out for potential memory leaks. ## Changelog [Internal] Reviewed By: NickGerleman Differential Revision: D73688009
rozele
force-pushed
the
export-D73688009
branch
2 times, most recently
from
35e0782 to
3ffada8
Compare
rozele
added a commit
to rozele/react-native-macos
that referenced
this pull request
May 5, 2025
Summary: In some apps, we spend a non-trivial amount of time calling ShadowNode destructors on the UI thread. A simple way to avoid stalling the UI thread is to move the `baseRevision_` instance to a data structure that is cleared on a background thread, so it's tree of ShadowNode shared_ptrs are released (and in most cases destroyed) on the background thread. Rather than using std::thread, this change introduces the LowPriorityExecutor abstraction that should be supplied by host platforms. The implementation of this LowPriorityExecutor for each platform is as follows: - iOS: uses dispatch_async to a low priority dispatch queue - Android: uses a pthread with SCHED_OTHER and priority = 19 Moving the ShadowTreeRevision into a lambda capture and punting the lambda to the LowPriorityExecutor moves the destructor calls of the ShadowNodes to the host platform implementation of the LowPriorityExecutor. This change is also guarded by a feature flag so we can keep an eye out for potential memory leaks. ## Changelog [Internal] Reviewed By: NickGerleman Differential Revision: D73688009
|
This pull request was exported from Phabricator. Differential Revision: D73688009 |
rozele
added a commit
to rozele/react-native-macos
that referenced
this pull request
May 5, 2025
Summary: Pull Request resolved: facebook#50997 In some apps, we spend a non-trivial amount of time calling ShadowNode destructors on the UI thread. A simple way to avoid stalling the UI thread is to move the `baseRevision_` instance to a data structure that is cleared on a background thread, so it's tree of ShadowNode shared_ptrs are released (and in most cases destroyed) on the background thread. Rather than using std::thread, this change introduces the LowPriorityExecutor abstraction that should be supplied by host platforms. The implementation of this LowPriorityExecutor for each platform is as follows: - iOS: uses dispatch_async to a low priority dispatch queue - Android: uses a pthread with SCHED_OTHER and priority = 19 Moving the ShadowTreeRevision into a lambda capture and punting the lambda to the LowPriorityExecutor moves the destructor calls of the ShadowNodes to the host platform implementation of the LowPriorityExecutor. This change is also guarded by a feature flag so we can keep an eye out for potential memory leaks. ## Changelog [Internal] Reviewed By: NickGerleman Differential Revision: D73688009
Summary: In some apps, we spend a non-trivial amount of time calling ShadowNode destructors on the UI thread. A simple way to avoid stalling the UI thread is to move the `baseRevision_` instance to a data structure that is cleared on a background thread, so it's tree of ShadowNode shared_ptrs are released (and in most cases destroyed) on the background thread. Rather than using std::thread, this change introduces the LowPriorityExecutor abstraction that should be supplied by host platforms. The implementation of this LowPriorityExecutor for each platform is as follows: - iOS: uses dispatch_async to a low priority dispatch queue - Android: uses a pthread with SCHED_OTHER and priority = 19 Moving the ShadowTreeRevision into a lambda capture and punting the lambda to the LowPriorityExecutor moves the destructor calls of the ShadowNodes to the host platform implementation of the LowPriorityExecutor. This change is also guarded by a feature flag so we can keep an eye out for potential memory leaks. ## Changelog [Internal] Reviewed By: NickGerleman Differential Revision: D73688009
|
This pull request was exported from Phabricator. Differential Revision: D73688009 |
|
This pull request was successfully merged by @rozele in d9823d8 When will my fix make it into a release? | How to file a pick request? |
|
This pull request has been merged in d9823d8. |
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.
Summary:
In some apps, we spend a non-trivial amount of time calling ShadowNode destructors on the UI thread.
A simple way to avoid stalling the UI thread is to move the
baseRevision_instance to a data structure that is cleared on a background thread, so it's tree of ShadowNode shared_ptrs are released (and in most cases destroyed) on the background thread.To minimize thread initialization costs, this change adds an AsyncDestructor helper that uses a single background thread and a queue of to-be-destroyed objects that are released in a run loop.
This change is also guarded by a feature flag so we can keep an eye out for potential memory leaks.
Changelog
[Internal]
Differential Revision: D73688009