From 971cda8794fc7b3ee5bac6c37581c444e88a62ad Mon Sep 17 00:00:00 2001 From: Marc Horowitz Date: Mon, 26 Sep 2016 16:01:38 -0700 Subject: [PATCH] Move thread jump for js loading into NativeToJSBridge, out of platform code Reviewed By: javache Differential Revision: D3906009 fbshipit-source-id: b9782a6c209e3c1626899dac7fd50233cdef87f3 --- .../react/XReactInstanceManagerImpl.java | 31 ++----------------- .../react/cxxbridge/CatalystInstanceImpl.java | 8 ++--- ReactCommon/cxxreact/Instance.cpp | 7 ++--- ReactCommon/cxxreact/JSCExecutor.cpp | 7 ++++- ReactCommon/cxxreact/NativeToJsBridge.cpp | 15 ++++----- ReactCommon/cxxreact/NativeToJsBridge.h | 22 +++++-------- 6 files changed, 28 insertions(+), 62 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/XReactInstanceManagerImpl.java b/ReactAndroid/src/main/java/com/facebook/react/XReactInstanceManagerImpl.java index 3802b2b23..91e8f0c95 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/XReactInstanceManagerImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/XReactInstanceManagerImpl.java @@ -18,8 +18,6 @@ import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.concurrent.Callable; -import java.util.concurrent.ExecutionException; import android.app.Activity; import android.app.Application; @@ -924,33 +922,8 @@ import static com.facebook.systrace.Systrace.TRACE_TAG_REACT_JAVA_BRIDGE; catalystInstance.addBridgeIdleDebugListener(mBridgeIdleDebugListener); } - ReactMarker.logMarker(RUN_JS_BUNDLE_START); - try { - catalystInstance.getReactQueueConfiguration().getJSQueueThread().callOnQueue( - new Callable() { - @Override - public Void call() throws Exception { - reactContext.initializeWithInstance(catalystInstance); - - Systrace.beginSection(TRACE_TAG_REACT_JAVA_BRIDGE, "runJSBundle"); - try { - catalystInstance.runJSBundle(); - } finally { - Systrace.endSection(TRACE_TAG_REACT_JAVA_BRIDGE); - ReactMarker.logMarker(RUN_JS_BUNDLE_END); - } - return null; - } - }).get(); - } catch (InterruptedException e) { - throw new RuntimeException(e); - } catch (ExecutionException e) { - if (e.getCause() instanceof RuntimeException) { - throw (RuntimeException) e.getCause(); - } else { - throw new RuntimeException(e); - } - } + reactContext.initializeWithInstance(catalystInstance); + catalystInstance.runJSBundle(); return reactContext; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/CatalystInstanceImpl.java b/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/CatalystInstanceImpl.java index 7bef0a38b..bd9d0697b 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/CatalystInstanceImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/CatalystInstanceImpl.java @@ -167,14 +167,14 @@ public class CatalystInstanceImpl implements CatalystInstance { @Override public void runJSBundle() { - // This should really be done when we post the task that runs the JS bundle - // (don't even need to wait for it to finish). Since that is currently done - // synchronously, marking it here is fine. - mAcceptCalls = true; Assertions.assertCondition(!mJSBundleHasLoaded, "JS bundle was already loaded!"); mJSBundleHasLoaded = true; // incrementPendingJSCalls(); mJSBundleLoader.loadScript(CatalystInstanceImpl.this); + // Loading the bundle is queued on the JS thread, but may not have + // run yet. It's save to set this here, though, since any work it + // gates will be queued on the JS thread behind the load. + mAcceptCalls = true; // This is registered after JS starts since it makes a JS call Systrace.registerListener(mTraceListener); } diff --git a/ReactCommon/cxxreact/Instance.cpp b/ReactCommon/cxxreact/Instance.cpp index eb5e31d94..42ddd74ee 100644 --- a/ReactCommon/cxxreact/Instance.cpp +++ b/ReactCommon/cxxreact/Instance.cpp @@ -49,8 +49,7 @@ void Instance::loadScriptFromString(std::unique_ptr string, callback_->incrementPendingJSCalls(); SystraceSection s("reactbridge_xplat_loadScriptFromString", "sourceURL", sourceURL); - // TODO mhorowitz: ReactMarker around loadApplicationScript - nativeToJsBridge_->loadApplicationScript(std::move(string), std::move(sourceURL)); + nativeToJsBridge_->loadApplication(nullptr, std::move(string), std::move(sourceURL)); } void Instance::loadScriptFromFile(const std::string& filename, @@ -90,8 +89,8 @@ void Instance::loadUnbundle(std::unique_ptr unbundle, std::string startupScriptSourceURL) { callback_->incrementPendingJSCalls(); SystraceSection s("reactbridge_xplat_setJSModulesUnbundle"); - nativeToJsBridge_->loadApplicationUnbundle(std::move(unbundle), std::move(startupScript), - std::move(startupScriptSourceURL)); + nativeToJsBridge_->loadApplication(std::move(unbundle), std::move(startupScript), + std::move(startupScriptSourceURL)); } bool Instance::supportsProfiling() { diff --git a/ReactCommon/cxxreact/JSCExecutor.cpp b/ReactCommon/cxxreact/JSCExecutor.cpp index 9b83686ed..e6fe99d0a 100644 --- a/ReactCommon/cxxreact/JSCExecutor.cpp +++ b/ReactCommon/cxxreact/JSCExecutor.cpp @@ -271,6 +271,8 @@ void JSCExecutor::loadApplicationScript( return loadApplicationScript(std::move(jsScriptBigString), sourceURL); } + ReactMarker::logMarker("RUN_JS_BUNDLE_START"); + if (flags & UNPACKED_BC_CACHE) { configureJSCBCCache(m_context, bundlePath); } @@ -290,6 +292,7 @@ void JSCExecutor::loadApplicationScript( flush(); ReactMarker::logMarker("CREATE_REACT_CONTEXT_END"); + ReactMarker::logMarker("RUN_JS_BUNDLE_END"); } #endif @@ -303,6 +306,8 @@ void JSCExecutor::loadApplicationScript(std::unique_ptr scrip "JSCExecutor::loadApplicationScript-createExpectingAscii"); #endif + ReactMarker::logMarker("RUN_JS_BUNDLE_START"); + ReactMarker::logMarker("loadApplicationScript_startStringConvert"); String jsScript = jsStringFromBigString(*script); ReactMarker::logMarker("loadApplicationScript_endStringConvert"); @@ -313,11 +318,11 @@ void JSCExecutor::loadApplicationScript(std::unique_ptr scrip String jsSourceURL(sourceURL.c_str()); evaluateScript(m_context, jsScript, jsSourceURL); - bindBridge(); flush(); ReactMarker::logMarker("CREATE_REACT_CONTEXT_END"); + ReactMarker::logMarker("RUN_JS_BUNDLE_END"); } void JSCExecutor::setJSModulesUnbundle(std::unique_ptr unbundle) { diff --git a/ReactCommon/cxxreact/NativeToJsBridge.cpp b/ReactCommon/cxxreact/NativeToJsBridge.cpp index 130ccd215..00fa82829 100644 --- a/ReactCommon/cxxreact/NativeToJsBridge.cpp +++ b/ReactCommon/cxxreact/NativeToJsBridge.cpp @@ -118,12 +118,6 @@ NativeToJsBridge::~NativeToJsBridge() { "NativeToJsBridge::destroy() must be called before deallocating the NativeToJsBridge!"; } -void NativeToJsBridge::loadApplicationScript(std::unique_ptr script, - std::string sourceURL) { - // TODO(t11144533): Add assert that we are on the correct thread - m_mainExecutor->loadApplicationScript(std::move(script), std::move(sourceURL)); -} - void NativeToJsBridge::loadOptimizedApplicationScript( std::string bundlePath, std::string sourceURL, @@ -138,18 +132,21 @@ void NativeToJsBridge::loadOptimizedApplicationScript( }); } -void NativeToJsBridge::loadApplicationUnbundle( +void NativeToJsBridge::loadApplication( std::unique_ptr unbundle, std::unique_ptr startupScript, std::string startupScriptSourceURL) { runOnExecutorQueue( m_mainExecutorToken, - [unbundle=folly::makeMoveWrapper(std::move(unbundle)), + [unbundleWrap=folly::makeMoveWrapper(std::move(unbundle)), startupScript=folly::makeMoveWrapper(std::move(startupScript)), startupScriptSourceURL=std::move(startupScriptSourceURL)] (JSExecutor* executor) mutable { - executor->setJSModulesUnbundle(unbundle.move()); + auto unbundle = unbundleWrap.move(); + if (unbundle) { + executor->setJSModulesUnbundle(std::move(unbundle)); + } executor->loadApplicationScript(std::move(*startupScript), std::move(startupScriptSourceURL)); }); diff --git a/ReactCommon/cxxreact/NativeToJsBridge.h b/ReactCommon/cxxreact/NativeToJsBridge.h index e95367378..429291375 100644 --- a/ReactCommon/cxxreact/NativeToJsBridge.h +++ b/ReactCommon/cxxreact/NativeToJsBridge.h @@ -78,11 +78,14 @@ public: void invokeCallback(ExecutorToken executorToken, double callbackId, folly::dynamic&& args); /** - * Starts the JS application from an "bundle", i.e. a JavaScript file that - * contains code for all modules and a runtime that resolves and - * executes modules. + * Starts the JS application. If unbundle is non-null, then it is + * used to fetch JavaScript modules as individual scripts. + * Otherwise, the script is assumed to include all the modules. */ - void loadApplicationScript(std::unique_ptr script, std::string sourceURL); + void loadApplication( + std::unique_ptr unbundle, + std::unique_ptr startupCode, + std::string sourceURL); /** * Similar to loading a "bundle", but instead of passing js source this method accepts @@ -90,17 +93,6 @@ public: */ void loadOptimizedApplicationScript(std::string bundlePath, std::string sourceURL, int flags); - /** - * An "unbundle" is a backend that stores and injects JavaScript modules as - * individual scripts, rather than bundling all of them into a single scrupt. - * - * Loading an unbundle means setting the storage backend and executing the - * startup script. - */ - void loadApplicationUnbundle( - std::unique_ptr unbundle, - std::unique_ptr startupCode, - std::string sourceURL); void setGlobalVariable(std::string propName, std::unique_ptr jsonValue); void* getJavaScriptContext(); bool supportsProfiling();