Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[iOS] Fixes main thread stuck when reload in bridgeless mode #45486

Closed
wants to merge 3 commits into from
Closed

[iOS] Fixes main thread stuck when reload in bridgeless mode #45486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6e32378
[iOS] Fixes main thread stuck when reload in bridgeless mode
zhongwuzw Jul 17, 2024
586c91f
Add missed nil set
zhongwuzw Jul 17, 2024
ff282eb
Call start surface on bufferedRuntime
zhongwuzw Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,8 @@ RuntimeExecutor ReactInstance::getUnbufferedRuntimeExecutor() noexcept {

// This BufferedRuntimeExecutor ensures that the main JS bundle finished
// execution before any JS queued into it from C++ are executed. Use
// getBufferedRuntimeExecutor() instead if you do not need the main JS bundle to
// have finished. e.g. setting global variables into JS runtime.
// getUnbufferedRuntimeExecutor() instead if you do not need the main JS bundle
// to have finished. e.g. setting global variables into JS runtime.
RuntimeExecutor ReactInstance::getBufferedRuntimeExecutor() noexcept {
return [weakBufferedRuntimeExecutor_ =
std::weak_ptr<BufferedRuntimeExecutor>(bufferedRuntimeExecutor_)](
Expand Down
54 changes: 4 additions & 50 deletions packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTHost.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,11 +104,6 @@ @implementation RCTHost {

NSDictionary *_launchOptions;

// All the surfaces that need to be started after main bundle execution
NSMutableArray<RCTFabricSurface *> *_surfaceStartBuffer;
std::mutex _surfaceStartBufferMutex;

RCTInstanceInitialBundleLoadCompletionBlock _onInitialBundleLoad;
std::vector<__weak RCTFabricSurface *> _attachedSurfaces;

RCTModuleRegistry *_moduleRegistry;
Expand Down Expand Up @@ -152,7 +147,6 @@ - (instancetype)initWithBundleURLProvider:(RCTHostBundleURLProvider)provider
if (self = [super init]) {
_hostDelegate = hostDelegate;
_turboModuleManagerDelegate = turboModuleManagerDelegate;
_surfaceStartBuffer = [NSMutableArray new];
_bundleManager = [RCTBundleManager new];
_moduleRegistry = [RCTModuleRegistry new];
_jsEngineProvider = [jsEngineProvider copy];
Expand Down Expand Up @@ -185,33 +179,6 @@ - (instancetype)initWithBundleURLProvider:(RCTHostBundleURLProvider)provider
andSetter:bundleURLSetter
andDefaultGetter:defaultBundleURLGetter];

/**
* TODO (T108401473) Remove _onInitialBundleLoad, because it was initially
* introduced to start surfaces after the main JSBundle was fully executed.
* It is no longer needed because Fabric now dispatches all native-to-JS calls
* onto the JS thread, including JS calls to start Fabric surfaces.
* JS calls should be dispatched using the BufferedRuntimeExecutor, which can buffer
* JS calls before the main JSBundle finishes execution, and execute them after.
*/
_onInitialBundleLoad = ^{
RCTHost *strongSelf = weakSelf;
if (!strongSelf) {
return;
}

NSArray<RCTFabricSurface *> *unstartedSurfaces = @[];

{
std::lock_guard<std::mutex> guard{strongSelf->_surfaceStartBufferMutex};
unstartedSurfaces = [NSArray arrayWithArray:strongSelf->_surfaceStartBuffer];
strongSelf->_surfaceStartBuffer = nil;
}

for (RCTFabricSurface *surface in unstartedSurfaces) {
[surface start];
}
};

// Listen to reload commands
dispatch_async(dispatch_get_main_queue(), ^{
RCTRegisterReloadCommandListener(self);
Expand Down Expand Up @@ -261,7 +228,7 @@ - (void)start
jsRuntimeFactory:[self _provideJSEngine]
bundleManager:_bundleManager
turboModuleManagerDelegate:_turboModuleManagerDelegate
onInitialBundleLoad:_onInitialBundleLoad
onInitialBundleLoad:nil
moduleRegistry:_moduleRegistry
parentInspectorTarget:_inspectorTarget.get()
launchOptions:_launchOptions];
Expand All @@ -278,15 +245,7 @@ - (RCTFabricSurface *)createSurfaceWithModuleName:(NSString *)moduleName
surface.surfaceHandler.setDisplayMode(displayMode);
[self _attachSurface:surface];

{
std::lock_guard<std::mutex> guard{_surfaceStartBufferMutex};
if (_surfaceStartBuffer) {
[_surfaceStartBuffer addObject:surface];
return surface;
}
}

[surface start];
[_instance callFunctionOnBufferedRumtimeExecutor:[surface](facebook::jsi::Runtime &_) { [surface start]; }];
return surface;
}

Expand Down Expand Up @@ -320,24 +279,19 @@ - (void)didReceiveReloadCommand
[self _setBundleURL:_bundleURLProvider()];
}

// Ensure all attached surfaces are restarted after reload
{
std::lock_guard<std::mutex> guard{_surfaceStartBufferMutex};
_surfaceStartBuffer = [NSMutableArray arrayWithArray:[self _getAttachedSurfaces]];
}

_instance = [[RCTInstance alloc] initWithDelegate:self
jsRuntimeFactory:[self _provideJSEngine]
bundleManager:_bundleManager
turboModuleManagerDelegate:_turboModuleManagerDelegate
onInitialBundleLoad:_onInitialBundleLoad
onInitialBundleLoad:nil
moduleRegistry:_moduleRegistry
parentInspectorTarget:_inspectorTarget.get()
launchOptions:_launchOptions];
[_hostDelegate hostDidStart:self];

for (RCTFabricSurface *surface in [self _getAttachedSurfaces]) {
[surface resetWithSurfacePresenter:self.surfacePresenter];
[_instance callFunctionOnBufferedRumtimeExecutor:[surface](facebook::jsi::Runtime &_) { [surface start]; }];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small typo "rumtime"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good catch, I would fix it in my next PR. :)

}
}

Expand Down
1 change: 1 addition & 0 deletions packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTInstance.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ typedef void (^_Null_unspecified RCTInstanceInitialBundleLoadCompletionBlock)();
launchOptions:(nullable NSDictionary *)launchOptions;

- (void)callFunctionOnJSModule:(NSString *)moduleName method:(NSString *)method args:(NSArray *)args;
- (void)callFunctionOnBufferedRumtimeExecutor:(std::function<void(facebook::jsi::Runtime &runtime)> &&)executor;

- (void)registerSegmentWithId:(NSNumber *)segmentId path:(NSString *)path;

Expand Down
9 changes: 9 additions & 0 deletions packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTInstance.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,15 @@ - (void)_attachBridgelessAPIsToModule:(id<RCTTurboModule>)module
}
}

- (void)callFunctionOnBufferedRumtimeExecutor:(std::function<void(facebook::jsi::Runtime &)> &&)executor
{
self->_reactInstance->getBufferedRuntimeExecutor()([=](jsi::Runtime &runtime) {
if (executor) {
executor(runtime);
}
});
}

- (void)handleBundleLoadingError:(NSError *)error
{
if (!_valid) {
Expand Down
Loading