Split loadApplicationScript into initializeRuntime and loadBundle #27844

simka · 2020-01-23T10:28:19Z

Summary

This is the first of three PRs related to enabling multi-bundle support in React Native. More details, motivation and reasoning behind it can be found in RFC here.

Logic responsible for installing globals was pulled out from loadApplicationScript to initializeRuntime since it should be ran only once, what was left was renamed to loadBundle.

It's based on @dratwas work from here, but applied to current master to avoid rebasing 3-months old branch and issues that come with that.

Changelog

[Internal] [Changed] - split loadApplicationScript into initializeRuntime and loadBundle to enable multi-bundle support in the future

Test Plan

Initialized new RN app with CLI, set RN to build from source and verified the still app builds and runs OK using code from this branch.

ejanzer

I originally suggested making this change if you were going to be splitting up the work that's currently done in loadApplicationScript into two functions, to avoid doing unnecessary initialization work on each subsequent bundle load. However, initializeRuntime is empty here - are you planning on adding something in a future PR? If not, then I don't think this change is needed, right?

zamotany · 2020-01-31T14:02:41Z

@ejanzer
initializeRuntime is only empty for the specific executor - RCTObjcExecutor. For other executors like JSIExecutor and ProxyExecutor, initializeRuntime will contain a logic, that should ideally be run only once. We've tried to more some of the logic (that made sense) out from RCTObjcExecutor constructors to it's initializeRuntime, but the app was crashing.

joeblynch · 2020-02-10T21:28:30Z

Hey @ejanzer, thanks for taking a look at this PR! Based on @zamotany's comment, do you think this PR is still needed? Also just a heads up, the other PRs that split out the work from the original RFC are up here and here.

ejanzer

@joeblynch @zamotany Sorry for the late response. Yes, that makes sense - I missed the actual implementations in ProxyExecutor and JSIExecutor when I looked at this. I think this looks good to me, just one change I'd suggest before we try to merge - I think renaming the markers will break perf logging, so we should probably keep those the same.

Thanks for your work on this!

ejanzer · 2020-02-10T22:11:37Z

ReactCommon/cxxreact/Instance.cpp

@@ -48,7 +48,8 @@ void Instance::initializeBridge(
  jsQueue->runOnQueueSync([this, &jsef, jsQueue]() mutable {
    nativeToJsBridge_ = std::make_unique<NativeToJsBridge>(
        jsef.get(), moduleRegistry_, jsQueue, callback_);
-
+    // TODO investigate why it has to be async


Why what has to be async? InitializeRuntime?

I think this is the old leftover, where we make the initial drafts of changes for RN 0.60. When implementing it, we've noticed that the code in NativeToJsBridge::initializeRuntime has to be run using runOnExecutorQueue. We haven't investigated more why we have to do it like that, since the code was working as expected and was crashing if runOnExecutorQueue was not used. This comment can be discarded and we'll remove it.

With some versions of JSC, if you create the runtime on one thread, and then use it on another, GC bugs will cause crashes. That's why this has to be done this way. It's not pretty, but there's not much else to be done if you want to use older versions of iOS. I'm not sure if the android JSC has this bug or not.

ejanzer · 2020-02-10T22:14:33Z

ReactCommon/cxxreact/JSExecutor.h

@@ -53,10 +53,14 @@ class JSExecutorFactory {

 class RN_EXPORT JSExecutor {
 public:
+  /**
+   * Sets all globals in the JS context.


Maybe something like: "Prepares the JS runtime for React Native by installing global variables. Called once before any JS is evaluated."

ejanzer · 2020-02-10T22:18:55Z

ReactAndroid/src/main/java/com/facebook/react/bridge/ReactMarkerConstants.java

@@ -59,8 +59,8 @@
  UNPACKING_JS_BUNDLE_LOADER_CHECK_END,
  UNPACKING_JS_BUNDLE_LOADER_EXTRACTED,
  UNPACKING_JS_BUNDLE_LOADER_BLOCKED,
-  loadApplicationScript_startStringConvert,
-  loadApplicationScript_endStringConvert,
+  loadBundle_startStringConvert,


This is the only thing that I'm a little bit concerned about - renaming our perf markers could be a breaking change for our perf instrumentation. I think it might be better to keep these the same, even though they won't match the function name anymore.

Totally valid concert, we'll revert that change.

joeblynch · 2020-02-11T01:28:58Z

Thank you @ejanzer for reviewing these PRs!

facebook-github-bot

@ejanzer has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

grabbou · 2020-02-26T13:51:07Z

Hey @ejanzer, is there any update on this? Seems to be taking a while, could be due to some internal merge conflicts.

react-native-bot · 2020-04-02T00:55:05Z

This pull request was successfully merged by @simka in 6f627f6.

^{When will my fix make it into a release? | Upcoming Releases}

…cebook#27844) Summary: This is the first of three PRs related to enabling multi-bundle support in React Native. More details, motivation and reasoning behind it can be found in RFC [here](react-native-community/discussions-and-proposals#152). Logic responsible for installing globals was pulled out from `loadApplicationScript` to `initializeRuntime` since it should be ran only once, what was left was renamed to `loadBundle`. It's based on dratwas work from [here](https://github.com/callstack/react-native/tree/feat/multibundle/split-load-application), but applied to current `master` to avoid rebasing 3-months old branch and issues that come with that. ## Changelog [Internal] [Changed] - split `loadApplicationScript` into `initializeRuntime` and `loadBundle` to enable multi-bundle support in the future Pull Request resolved: facebook#27844 Test Plan: Initialized new RN app with CLI, set RN to build from source and verified the still app builds and runs OK using code from this branch. Reviewed By: rickhanlonii Differential Revision: D19888605 Pulled By: ejanzer fbshipit-source-id: 24ace48ffe8978796591fe7c6cf53a61b127cce6

…ndle (facebook#27844)" This reverts commit 6f627f6.

* Add nullability checks (#704) * Update RCTCxxBridge.mm * add nullability checks * Revert "Split loadApplicationScript into initializeRuntime and loadBundle (facebook#27844)" This reverts commit 6f627f6. * pod fixes * Revert "Split loadApplicationScript into initializeRuntime and loadBundle (facebook#27844)" This reverts commit 6f627f6. * pod fixes * pod update * pod install * fix libevents and flipper dependencies * flipper updates * disable sonar kit * Revert "Split loadApplicationScript into initializeRuntime and loadBundle (facebook#27844)" This reverts commit 6f627f6. * pod fixes * flipper updates * disable sonar kit * pod install

feat: split loadApplicationScript into initializeRuntime and loadBundle

767d217

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 23, 2020

simka marked this pull request as ready for review January 23, 2020 10:44

simka requested a review from mhorowitz as a code owner January 23, 2020 10:44

ejanzer reviewed Jan 30, 2020

View reviewed changes

ejanzer mentioned this pull request Jan 30, 2020

Host/Instance Management in RN react-native-community/discussions-and-proposals#183

Closed

ejanzer suggested changes Feb 10, 2020

View reviewed changes

simka added 4 commits February 11, 2020 11:49

improve JSExecutor comment

e830054

revert perf markers naming change

9f61ca2

remove unnecessary comment

5951351

revert marker changes in JReactMarker

d95a874

ejanzer approved these changes Feb 13, 2020

View reviewed changes

facebook-github-bot reviewed Feb 13, 2020

View reviewed changes

facebook-github-bot closed this in 6f627f6 Apr 2, 2020

react-native-bot added the Merged This PR has been merged. label Apr 2, 2020

HeyImChris added a commit to HeyImChris/react-native-macos that referenced this pull request May 20, 2021

Revert "Split loadApplicationScript into initializeRuntime and loadBu…

c1c88f0

…ndle (facebook#27844)" This reverts commit 6f627f6.

HeyImChris added a commit to HeyImChris/react-native-macos that referenced this pull request May 25, 2021

Revert "Split loadApplicationScript into initializeRuntime and loadBu…

36501ac

…ndle (facebook#27844)" This reverts commit 6f627f6.

HeyImChris added a commit to HeyImChris/react-native-macos that referenced this pull request Jun 1, 2021

Revert "Split loadApplicationScript into initializeRuntime and loadBu…

6efc86a

…ndle (facebook#27844)" This reverts commit 6f627f6.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split loadApplicationScript into initializeRuntime and loadBundle #27844

Split loadApplicationScript into initializeRuntime and loadBundle #27844

simka commented Jan 23, 2020

ejanzer left a comment

zamotany commented Jan 31, 2020

joeblynch commented Feb 10, 2020

ejanzer left a comment

ejanzer Feb 10, 2020

zamotany Feb 12, 2020

mhorowitz Feb 12, 2020

ejanzer Feb 10, 2020

ejanzer Feb 10, 2020

zamotany Feb 12, 2020

joeblynch commented Feb 11, 2020

facebook-github-bot left a comment

grabbou commented Feb 26, 2020

react-native-bot commented Apr 2, 2020

Split loadApplicationScript into initializeRuntime and loadBundle #27844

Split loadApplicationScript into initializeRuntime and loadBundle #27844

Conversation

simka commented Jan 23, 2020

Summary

Changelog

Test Plan

ejanzer left a comment

Choose a reason for hiding this comment

zamotany commented Jan 31, 2020

joeblynch commented Feb 10, 2020

ejanzer left a comment

Choose a reason for hiding this comment

ejanzer Feb 10, 2020

Choose a reason for hiding this comment

zamotany Feb 12, 2020

Choose a reason for hiding this comment

mhorowitz Feb 12, 2020

Choose a reason for hiding this comment

ejanzer Feb 10, 2020

Choose a reason for hiding this comment

ejanzer Feb 10, 2020

Choose a reason for hiding this comment

zamotany Feb 12, 2020

Choose a reason for hiding this comment

joeblynch commented Feb 11, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

grabbou commented Feb 26, 2020

react-native-bot commented Apr 2, 2020