-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
Initialize the RuntimeScheduler and TurboModule bindings via RuntimeExecutor #12402
base: main
Are you sure you want to change the base?
Conversation
11f936d
to
e4aedb9
Compare
e4aedb9
to
11d6898
Compare
longLivedObjectCollection = m_longLivedObjectCollection]() { | ||
// Use RuntimeExecutor so non-ABI JSExecutorFactory instances passed in through DevSettings | ||
// can use the same logic | ||
runtimeExecutor([propertyBag, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously we ran this code synchronously.
As comment used to say that it enabled us to have the TurboModuleManager fully initialized by the time we exit the method.
This PR changes it to run asynchronously in a different thread. Thus, the TurboModuleManager maybe not initialized by the time we exit this method.
My concern is that this code may affect scenarios where we previously relied on the TurboModuleManager be initialized.
Should we run the runtimeExecutor synchronously here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This deadlocks when trying to run synchronously on the RuntimeExecutor, so I left it as async for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think perhaps we should do this conditionally. We can keep this "as is" if using ABI JSI, and switch to the async RuntimeExecutor only if using the non-ABI JSI option.
…xecutor In order to minimize the dependency on the JSI runtime holder, this switches the initialization of TurboModules and RuntimeScheduler bindings to use the RuntimeExecutor, which hands us back the Runtime instance.
11d6898
to
39895bb
Compare
Description
Type of Change
Erase all that don't apply.
Why
In order to minimize the dependency on the JSI runtime holder, this switches the initialization of TurboModules and RuntimeScheduler bindings to use the RuntimeExecutor, which hands us back the Runtime instance.
Testing
Ran Playground, TurboModules still appear to initialize correctly (or else the UIManager wouldn't be able to reload).
Changelog
Should this change be included in the release notes: no
Microsoft Reviewers: Open in CodeFlow