From e1758411b9fc6a700c1f2dfd1e599f86f5938d1b Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Wed, 30 Oct 2019 18:05:45 -0700 Subject: [PATCH] Remove "bytecodeFileName" concept from "microsoft/react-native" bytecodeFileName is used excusively by ChakraExecutor in react-native-windows, and diverges us from upstream. Remove the concept in favor of passing bytecodeFileName directly to the ChakraJsExecutorFactory at creation time. We currently use a single bytecode file per-instance , so we see no loss of functionality when moving to a 1:1 relationship between executor and bytecode file. Locally validated that Android NDK libs still build. Verified we can build react-native-windows after some small changes that must be commited separately. --- React/CxxBridge/RCTCxxBridge.mm | 4 ++-- React/CxxBridge/RCTObjcExecutor.mm | 3 +-- .../react/v8executor/InstanceManager.cpp | 4 ++-- .../jni/react/jni/CatalystInstanceImpl.cpp | 4 ++-- .../main/jni/react/jni/InstanceManager.cpp | 4 ++-- .../src/main/jni/react/jni/ProxyExecutor.cpp | 3 +-- .../src/main/jni/react/jni/ProxyExecutor.h | 3 +-- ReactCommon/cxxreact/Instance.cpp | 23 ++++++++----------- ReactCommon/cxxreact/Instance.h | 9 +++----- ReactCommon/cxxreact/JSExecutor.h | 3 +-- ReactCommon/cxxreact/NativeToJsBridge.cpp | 15 ++++-------- ReactCommon/cxxreact/NativeToJsBridge.h | 6 ++--- ReactCommon/cxxreact/V8Executor.cpp | 4 ++-- ReactCommon/cxxreact/V8Executor.h | 3 +-- .../jsiexecutor/jsireact/JSIExecutor.cpp | 3 +-- .../jsiexecutor/jsireact/JSIExecutor.h | 3 +-- 16 files changed, 37 insertions(+), 57 deletions(-) diff --git a/React/CxxBridge/RCTCxxBridge.mm b/React/CxxBridge/RCTCxxBridge.mm index e3adc15e86c598..3f2e7600544c2e 100644 --- a/React/CxxBridge/RCTCxxBridge.mm +++ b/React/CxxBridge/RCTCxxBridge.mm @@ -1337,8 +1337,8 @@ - (void)executeApplicationScript:(NSData *)script sourceUrlStr.UTF8String, !async); } } else if (reactInstance) { - reactInstance->loadScriptFromString(std::make_unique(script), 0, - sourceUrlStr.UTF8String, !async, ""); // TODO(OSS Candidate ISS#2710739) + reactInstance->loadScriptFromString(std::make_unique(script), 0, // TODO(OSS Candidate ISS#2710739) + sourceUrlStr.UTF8String, !async); } else { std::string methodName = async ? "loadApplicationScript" : "loadApplicationScriptSync"; throw std::logic_error("Attempt to call " + methodName + ": on uninitialized bridge"); diff --git a/React/CxxBridge/RCTObjcExecutor.mm b/React/CxxBridge/RCTObjcExecutor.mm index c0e532bb4c1363..3ba442abd8b370 100644 --- a/React/CxxBridge/RCTObjcExecutor.mm +++ b/React/CxxBridge/RCTObjcExecutor.mm @@ -77,8 +77,7 @@ void loadApplicationScript( std::unique_ptr script, uint64_t /*scriptVersion*/, // TODO(OSS Candidate ISS#2710739) - std::string sourceURL, - std::string&& /*bytecodeFileName*/) override { // TODO(OSS Candidate ISS#2710739) + std::string sourceURL) override { RCTProfileBeginFlowEvent(); [m_jse executeApplicationScript:[NSData dataWithBytes:script->c_str() length:script->size()] sourceURL:[[NSURL alloc] diff --git a/ReactAndroid/src/main/java/com/facebook/react/v8executor/InstanceManager.cpp b/ReactAndroid/src/main/java/com/facebook/react/v8executor/InstanceManager.cpp index 3632fa4d6244e6..39056993326bd0 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/v8executor/InstanceManager.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/v8executor/InstanceManager.cpp @@ -78,7 +78,7 @@ std::shared_ptr CreateReactInstance( // Load from Assets. script = loadScriptFromAssets(assetManager, platformBundle.BundleUrl); } - instance->loadScriptFromString(std::move(script), platformBundle.Version, std::move(platformBundle.BundleUrl), true /*synchronously*/, "" /*bytecodeFileName*/); + instance->loadScriptFromString(std::move(script), platformBundle.Version, std::move(platformBundle.BundleUrl), true /*synchronously*/); } } @@ -94,7 +94,7 @@ std::shared_ptr CreateReactInstance( // Load from Assets. script = loadScriptFromAssets(assetManager, jsBundleFile); } - instance->loadScriptFromString(std::move(script), 0 /*bundleVersion*/, jsBundleFile, false, "" /*bytecodeFileName*/); + instance->loadScriptFromString(std::move(script), 0 /*bundleVersion*/, jsBundleFile, false); return instance; } diff --git a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp index e91471a5babcaa..943a3b225c545c 100644 --- a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp +++ b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp @@ -211,7 +211,7 @@ void CatalystInstanceImpl::jniLoadScriptFromAssets( } else if (Instance::isIndexedRAMBundle(&script)) { instance_->loadRAMBundleFromString(std::move(script), sourceURL); } else { - instance_->loadScriptFromString(std::move(script), 0 /*bundleVersion*/, sourceURL, loadSynchronously, "" /*bytecodeFileName*/); + instance_->loadScriptFromString(std::move(script), 0 /*bundleVersion*/, sourceURL, loadSynchronously); } } @@ -226,7 +226,7 @@ void CatalystInstanceImpl::jniLoadScriptFromFile(const std::string& fileName, [&fileName, &script]() { script = JSBigFileString::fromPath(fileName); }); - instance_->loadScriptFromString(std::move(script), 0 /*bundleVersion*/, sourceURL, loadSynchronously, "" /*bytecodeFileName*/); + instance_->loadScriptFromString(std::move(script), 0 /*bundleVersion*/, sourceURL, loadSynchronously); } } diff --git a/ReactAndroid/src/main/jni/react/jni/InstanceManager.cpp b/ReactAndroid/src/main/jni/react/jni/InstanceManager.cpp index 1437abaac2ec66..95806c4b1a9b55 100644 --- a/ReactAndroid/src/main/jni/react/jni/InstanceManager.cpp +++ b/ReactAndroid/src/main/jni/react/jni/InstanceManager.cpp @@ -79,7 +79,7 @@ std::shared_ptr CreateReactInstance( // Load from Assets. script = loadScriptFromAssets(assetManager, platformBundle.BundleUrl); } - instance->loadScriptFromString(std::move(script), platformBundle.Version, std::move(platformBundle.BundleUrl), true /*synchronously*/, "" /*bytecodeFileName*/); + instance->loadScriptFromString(std::move(script), platformBundle.Version, std::move(platformBundle.BundleUrl), true /*synchronously*/); } } @@ -95,7 +95,7 @@ std::shared_ptr CreateReactInstance( // Load from Assets. script = loadScriptFromAssets(assetManager, jsBundleFile); } - instance->loadScriptFromString(std::move(script), 0 /*bundleVersion*/, jsBundleFile, false, "" /*bytecodeFileName*/); + instance->loadScriptFromString(std::move(script), 0 /*bundleVersion*/, jsBundleFile, false); return instance; } diff --git a/ReactAndroid/src/main/jni/react/jni/ProxyExecutor.cpp b/ReactAndroid/src/main/jni/react/jni/ProxyExecutor.cpp index 2c2e7eac6c0742..4c11bbf0375ed4 100644 --- a/ReactAndroid/src/main/jni/react/jni/ProxyExecutor.cpp +++ b/ReactAndroid/src/main/jni/react/jni/ProxyExecutor.cpp @@ -52,8 +52,7 @@ ProxyExecutor::~ProxyExecutor() { void ProxyExecutor::loadApplicationScript( std::unique_ptr, uint64_t /*scriptVersion*/, - std::string sourceURL, - std::string&& /*bytecodeFileName*/) { + std::string sourceURL) { folly::dynamic nativeModuleConfig = folly::dynamic::array; diff --git a/ReactAndroid/src/main/jni/react/jni/ProxyExecutor.h b/ReactAndroid/src/main/jni/react/jni/ProxyExecutor.h index 7f49594ac5cfba..7ff8eab5b271d8 100644 --- a/ReactAndroid/src/main/jni/react/jni/ProxyExecutor.h +++ b/ReactAndroid/src/main/jni/react/jni/ProxyExecutor.h @@ -38,8 +38,7 @@ class ProxyExecutor : public JSExecutor { virtual void loadApplicationScript( std::unique_ptr script, uint64_t scriptVersion, - std::string sourceURL, - std::string&& bytecodeFileName) override; + std::string sourceURL) override; virtual void setBundleRegistry( std::unique_ptr bundle) override; virtual void registerBundle( diff --git a/ReactCommon/cxxreact/Instance.cpp b/ReactCommon/cxxreact/Instance.cpp index cf53b702c41a53..3b7d7067876b92 100644 --- a/ReactCommon/cxxreact/Instance.cpp +++ b/ReactCommon/cxxreact/Instance.cpp @@ -70,46 +70,43 @@ void Instance::initializeBridge( void Instance::loadApplication(std::unique_ptr bundleRegistry, std::unique_ptr bundle, uint64_t bundleVersion, // TODO(OSS Candidate ISS#2710739) - std::string bundleURL, - std::string&& bytecodeFileName) { // TODO(OSS Candidate ISS#2710739) + std::string bundleURL) { // TODO(OSS Candidate ISS#2710739) callback_->incrementPendingJSCalls(); SystraceSection s("Instance::loadApplication", "bundleURL", bundleURL); nativeToJsBridge_->loadApplication(std::move(bundleRegistry), std::move(bundle), bundleVersion, - std::move(bundleURL), std::move(bytecodeFileName)); + std::move(bundleURL)); } void Instance::loadApplicationSync(std::unique_ptr bundleRegistry, std::unique_ptr bundle, uint64_t bundleVersion, // TODO(OSS Candidate ISS#2710739) - std::string bundleURL, - std::string&& bytecodeFileName) { // TODO(OSS Candidate ISS#2710739) + std::string bundleURL) { std::unique_lock lock(m_syncMutex); m_syncCV.wait(lock, [this] { return m_syncReady; }); SystraceSection s("Instance::loadApplicationSync", "bundleURL", bundleURL); nativeToJsBridge_->loadApplicationSync(std::move(bundleRegistry), std::move(bundle), bundleVersion, - std::move(bundleURL), std::move(bytecodeFileName)); // TODO(OSS Candidate ISS#2710739) + std::move(bundleURL)); } void Instance::setSourceURL(std::string sourceURL) { callback_->incrementPendingJSCalls(); SystraceSection s("Instance::setSourceURL", "sourceURL", sourceURL); - nativeToJsBridge_->loadApplication(nullptr, nullptr, 0, std::move(sourceURL), "" /*bytecodeFileName*/); // TODO(OSS Candidate ISS#2710739) + nativeToJsBridge_->loadApplication(nullptr, nullptr, 0, std::move(sourceURL)); // TODO(OSS Candidate ISS#2710739) } void Instance::loadScriptFromString(std::unique_ptr bundleString, uint64_t bundleVersion, std::string bundleURL, // TODO(OSS Candidate ISS#2710739) - bool loadSynchronously, - std::string&& bytecodeFileName) { // TODO(OSS Candidate ISS#2710739) + bool loadSynchronously) { SystraceSection s("Instance::loadScriptFromString", "bundleURL", bundleURL); // TODO(OSS Candidate ISS#2710739) if (loadSynchronously) { - loadApplicationSync(nullptr, std::move(bundleString), bundleVersion, std::move(bundleURL), std::move(bytecodeFileName)); // TODO(OSS Candidate ISS#2710739) + loadApplicationSync(nullptr, std::move(bundleString), bundleVersion, std::move(bundleURL)); // TODO(OSS Candidate ISS#2710739) } else { - loadApplication(nullptr, std::move(bundleString), bundleVersion, std::move(bundleURL), std::move(bytecodeFileName)); // TODO(OSS Candidate ISS#2710739) + loadApplication(nullptr, std::move(bundleString), bundleVersion, std::move(bundleURL)); // TODO(OSS Candidate ISS#2710739) } } @@ -162,10 +159,10 @@ void Instance::loadRAMBundle(std::unique_ptr bundleRegistry, bool loadSynchronously) { if (loadSynchronously) { loadApplicationSync(std::move(bundleRegistry), std::move(startupScript), 0 /*bundleVersion*/, // TODO(OSS Candidate ISS#2710739) - std::move(startupScriptSourceURL), "" /*bytecodeFileName*/); // TODO(OSS Candidate ISS#2710739) + std::move(startupScriptSourceURL)); } else { loadApplication(std::move(bundleRegistry), std::move(startupScript), 0 /*bundleVersion*/, // TODO(OSS Candidate ISS#2710739) - std::move(startupScriptSourceURL), "" /*bytecodeFileName*/); // TODO(OSS Candidate ISS#2710739) + std::move(startupScriptSourceURL)); } } diff --git a/ReactCommon/cxxreact/Instance.h b/ReactCommon/cxxreact/Instance.h index ad8473b125ae71..cc0646d40a3624 100644 --- a/ReactCommon/cxxreact/Instance.h +++ b/ReactCommon/cxxreact/Instance.h @@ -63,8 +63,7 @@ class RN_EXPORT Instance { void setSourceURL(std::string sourceURL); virtual void loadScriptFromString(std::unique_ptr bundleString, - uint64_t bundleVersion, std::string bundleURL, bool loadSynchronously, - std::string&& bytecodeFileName); + uint64_t bundleVersion, std::string bundleURL, bool loadSynchronously); static bool isIndexedRAMBundle(const char *sourcePath); static bool isIndexedRAMBundle(std::unique_ptr* string); void loadRAMBundleFromString(std::unique_ptr script, const std::string& sourceURL); @@ -107,13 +106,11 @@ class RN_EXPORT Instance { virtual void loadApplication(std::unique_ptr bundleRegistry, std::unique_ptr bundle, uint64_t bundleVersion, // TODO(OSS Candidate ISS#2710739) - std::string bundleURL, - std::string&& bytecodeFileName); // TODO(OSS Candidate ISS#2710739) + std::string bundleURL); virtual void loadApplicationSync(std::unique_ptr bundleRegistry, std::unique_ptr bundle, uint64_t bundleVersion, // TODO(OSS Candidate ISS#2710739) - std::string bundleURL, - std::string&& bytecodeFileName); // TODO(OSS Candidate ISS#2710739) + std::string bundleURL); std::shared_ptr callback_; std::unique_ptr nativeToJsBridge_; diff --git a/ReactCommon/cxxreact/JSExecutor.h b/ReactCommon/cxxreact/JSExecutor.h index b6a08ad813690a..107d7b139a25ba 100644 --- a/ReactCommon/cxxreact/JSExecutor.h +++ b/ReactCommon/cxxreact/JSExecutor.h @@ -78,8 +78,7 @@ class RN_EXPORT JSExecutor { */ virtual void loadApplicationScript(std::unique_ptr script, uint64_t scriptVersion, // TODO(OSS Candidate ISS#2710739) - std::string sourceURL, - std::string&& bytecodeFileName) = 0; // TODO(OSS Candidate ISS#2710739) + std::string sourceURL) = 0; /** * Add an application "RAM" bundle registry diff --git a/ReactCommon/cxxreact/NativeToJsBridge.cpp b/ReactCommon/cxxreact/NativeToJsBridge.cpp index 7249d4c5550f25..fa06a2d1759308 100644 --- a/ReactCommon/cxxreact/NativeToJsBridge.cpp +++ b/ReactCommon/cxxreact/NativeToJsBridge.cpp @@ -106,16 +106,14 @@ void NativeToJsBridge::loadApplication( std::unique_ptr bundleRegistry, std::unique_ptr startupScript, uint64_t bundleVersion, // TODO(OSS Candidate ISS#2710739) - std::string startupScriptSourceURL, - std::string&& bytecodeFileName) { // TODO(OSS Candidate ISS#2710739) + std::string startupScriptSourceURL) { runOnExecutorQueue( [this, bundleRegistryWrap=folly::makeMoveWrapper(std::move(bundleRegistry)), startupScript=folly::makeMoveWrapper(std::move(startupScript)), bundleVersion, - startupScriptSourceURL=std::move(startupScriptSourceURL), - bytecodeFileName=std::move(bytecodeFileName)] + startupScriptSourceURL=std::move(startupScriptSourceURL)] (JSExecutor* executor) mutable { auto bundleRegistry = bundleRegistryWrap.move(); if (bundleRegistry) { @@ -124,8 +122,7 @@ void NativeToJsBridge::loadApplication( try { executor->loadApplicationScript(std::move(*startupScript), bundleVersion, // TODO(OSS Candidate ISS#2710739) - std::move(startupScriptSourceURL), - std::move(bytecodeFileName)); // TODO(OSS Candidate ISS#2710739) + std::move(startupScriptSourceURL)); } catch (...) { m_applicationScriptHasFailure = true; throw; @@ -137,16 +134,14 @@ void NativeToJsBridge::loadApplicationSync( std::unique_ptr bundleRegistry, std::unique_ptr startupScript, uint64_t bundleVersion, - std::string startupScriptSourceURL, - std::string&& bytecodeFileName) { + std::string startupScriptSourceURL) { if (bundleRegistry) { m_executor->setBundleRegistry(std::move(bundleRegistry)); } try { m_executor->loadApplicationScript(std::move(startupScript), bundleVersion, // TODO(OSS Candidate ISS#2710739) - std::move(startupScriptSourceURL), - std::move(bytecodeFileName)); // TODO(OSS Candidate ISS#2710739) + std::move(startupScriptSourceURL)); } catch (...) { m_applicationScriptHasFailure = true; throw; diff --git a/ReactCommon/cxxreact/NativeToJsBridge.h b/ReactCommon/cxxreact/NativeToJsBridge.h index e02cf192889d4d..0b7dae3d368102 100644 --- a/ReactCommon/cxxreact/NativeToJsBridge.h +++ b/ReactCommon/cxxreact/NativeToJsBridge.h @@ -68,14 +68,12 @@ class NativeToJsBridge { std::unique_ptr bundleRegistry, std::unique_ptr bundle, uint64_t bundleVersion, // TODO(OSS Candidate ISS#2710739) - std::string bundleURL, - std::string&& bytecodeFileName); // TODO(OSS Candidate ISS#2710739) + std::string bundleURL); void loadApplicationSync( std::unique_ptr bundleRegistry, std::unique_ptr bundle, uint64_t bundleVersion, // TODO(OSS Candidate ISS#2710739) - std::string bundleURL, - std::string&& bytecodeFileName); // TODO(OSS Candidate ISS#2710739) + std::string bundleURL); void registerBundle(uint32_t bundleId, const std::string& bundlePath); void setGlobalVariable(std::string propName, std::unique_ptr jsonValue); diff --git a/ReactCommon/cxxreact/V8Executor.cpp b/ReactCommon/cxxreact/V8Executor.cpp index 24835dc36f8c28..b05292d8c48cfa 100644 --- a/ReactCommon/cxxreact/V8Executor.cpp +++ b/ReactCommon/cxxreact/V8Executor.cpp @@ -368,9 +368,9 @@ static std::string simpleBasename(const std::string &path) { LOGV("V8Executor::simpleBasename exit"); } -void V8Executor::loadApplicationScript(std::unique_ptr script, uint64_t /*scriptVersion*/, std::string sourceURL, std::string&& bytecodeFileName) { +void V8Executor::loadApplicationScript(std::unique_ptr script, uint64_t /*scriptVersion*/, std::string sourceURL) { - LOGV("V8Executor::loadApplicationScript entry sourceURL = %s, bytecodeFileName = %s", sourceURL.c_str(), bytecodeFileName.c_str()); + LOGV("V8Executor::loadApplicationScript entry sourceURL = %s", sourceURL.c_str()); SystraceSection s("V8Executor::loadApplicationScript", "sourceURL", sourceURL); std::string scriptName = simpleBasename(sourceURL); ReactMarker::logTaggedMarker(ReactMarker::RUN_JS_BUNDLE_START, scriptName.c_str()); diff --git a/ReactCommon/cxxreact/V8Executor.h b/ReactCommon/cxxreact/V8Executor.h index 0e0bb1bf83c774..9748c2379e94cd 100644 --- a/ReactCommon/cxxreact/V8Executor.h +++ b/ReactCommon/cxxreact/V8Executor.h @@ -55,8 +55,7 @@ class RN_EXPORT V8Executor : public JSExecutor, public PrivateDataBase { virtual void loadApplicationScript( std::unique_ptr script, uint64_t scriptVersion, - std::string sourceURL, - std::string&& bytecodeFileName) override; + std::string sourceURL) override; virtual void registerBundle(uint32_t bundleId, const std::string& bundlePath) override; diff --git a/ReactCommon/jsiexecutor/jsireact/JSIExecutor.cpp b/ReactCommon/jsiexecutor/jsireact/JSIExecutor.cpp index 85aba642195b80..2533da8f443daa 100644 --- a/ReactCommon/jsiexecutor/jsireact/JSIExecutor.cpp +++ b/ReactCommon/jsiexecutor/jsireact/JSIExecutor.cpp @@ -70,8 +70,7 @@ JSIExecutor::JSIExecutor( void JSIExecutor::loadApplicationScript( std::unique_ptr script, uint64_t /*scriptVersion*/, // TODO(OSS Candidate ISS#2710739) - std::string sourceURL, - std::string&& /*bytecodeFileName*/) { // TODO(OSS Candidate ISS#2710739) + std::string sourceURL) { SystraceSection s("JSIExecutor::loadApplicationScript"); // TODO: check for and use precompiled HBC diff --git a/ReactCommon/jsiexecutor/jsireact/JSIExecutor.h b/ReactCommon/jsiexecutor/jsireact/JSIExecutor.h index 0fce13ccd7e4d0..acbaf579c11be5 100644 --- a/ReactCommon/jsiexecutor/jsireact/JSIExecutor.h +++ b/ReactCommon/jsiexecutor/jsireact/JSIExecutor.h @@ -77,8 +77,7 @@ class JSIExecutor : public JSExecutor { RuntimeInstaller runtimeInstaller); void loadApplicationScript(std::unique_ptr script, uint64_t scriptVersion, // TODO(OSS Candidate ISS#2710739) - std::string sourceURL, - std::string&& bytecodeFileName) override; // TODO(OSS Candidate ISS#2710739) + std::string sourceURL) override; void setBundleRegistry(std::unique_ptr) override; void registerBundle(uint32_t bundleId, const std::string &bundlePath) override;