From 1343313dc6fd2b8bc111ec4ccf03d4aa05fcfff7 Mon Sep 17 00:00:00 2001 From: Pieter De Baets Date: Thu, 23 May 2024 12:33:41 -0700 Subject: [PATCH] Invoke callableModule factory once (#44576) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/44576 Store callable modules as either a factory function or an object, so we can skip invoking the factory function for frequently accessed objects. Changelog: [Internal] Reviewed By: sammy-SC Differential Revision: D57338528 fbshipit-source-id: cd39ccbe7168c6f093a0e62d5880cbbcd5209c8e --- .../react/runtime/ReactInstance.cpp | 44 +++++++++++-------- .../ReactCommon/react/runtime/ReactInstance.h | 11 ++--- .../runtime/tests/cxx/ReactInstanceTest.cpp | 8 ++-- 3 files changed, 33 insertions(+), 30 deletions(-) diff --git a/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp b/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp index 7f51293d038..5a1d10fdaaa 100644 --- a/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp +++ b/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp @@ -232,20 +232,24 @@ void ReactInstance::loadScript( void ReactInstance::callFunctionOnModule( const std::string& moduleName, const std::string& methodName, - const folly::dynamic& args) { - // TODO (C++ 20): This code previously implicitly captured `this` in a [=] - // capture group. Was it meaning to pass modules_ by value? - bufferedRuntimeExecutor_->execute([=, this](jsi::Runtime& runtime) { + folly::dynamic&& args) { + bufferedRuntimeExecutor_->execute([this, + moduleName = moduleName, + methodName = methodName, + args = std::move(args)]( + jsi::Runtime& runtime) { SystraceSection s( "ReactInstance::callFunctionOnModule", "moduleName", moduleName, "methodName", methodName); - if (modules_.find(moduleName) == modules_.end()) { + auto it = callableModules_.find(moduleName); + if (it == callableModules_.end()) { std::ostringstream knownModules; int i = 0; - for (auto it = modules_.begin(); it != modules_.end(); it++, i++) { + for (it = callableModules_.begin(); it != callableModules_.end(); + it++, i++) { const char* space = (i > 0 ? ", " : " "); knownModules << space << it->first; } @@ -254,24 +258,25 @@ void ReactInstance::callFunctionOnModule( "Failed to call into JavaScript module method " + moduleName + "." + methodName + "(). Module has not been registered as callable. Registered callable JavaScript modules (n = " + - std::to_string(modules_.size()) + "):" + knownModules.str() + - ". Did you forget to call `RN$registerCallableModule`?"); + std::to_string(callableModules_.size()) + + "):" + knownModules.str() + + ". Did you forget to call `registerCallableModule`?"); } - auto module = modules_[moduleName]->factory.call(runtime).asObject(runtime); - auto method = module.getProperty(runtime, methodName.c_str()); - if (method.isUndefined()) { - throw jsi::JSError( - runtime, - "Failed to call into JavaScript module method " + moduleName + "." + - methodName + ". Module exists, but the method is undefined."); + if (std::holds_alternative(it->second)) { + auto module = + std::get(it->second).call(runtime).asObject(runtime); + it->second = std::move(module); } + auto& module = std::get(it->second); + auto method = module.getPropertyAsFunction(runtime, methodName.c_str()); + std::vector jsArgs; for (auto& arg : args) { jsArgs.push_back(jsi::valueFromDynamic(runtime, arg)); } - method.asObject(runtime).asFunction(runtime).callWithThis( + method.callWithThis( runtime, module, (const jsi::Value*)jsArgs.data(), jsArgs.size()); }); } @@ -367,13 +372,14 @@ void ReactInstance::initializeRuntime( } auto name = args[0].asString(runtime).utf8(runtime); if (!args[1].isObject() || - !args[1].asObject(runtime).isFunction(runtime)) { + !args[1].getObject(runtime).isFunction(runtime)) { throw jsi::JSError( runtime, "The second argument to registerCallableModule must be a function that returns the JS module."); } - modules_[name] = std::make_shared( - args[1].getObject(runtime).asFunction(runtime)); + callableModules_.emplace( + std::move(name), + args[1].getObject(runtime).getFunction(runtime)); return jsi::Value::undefined(); })); diff --git a/packages/react-native/ReactCommon/react/runtime/ReactInstance.h b/packages/react-native/ReactCommon/react/runtime/ReactInstance.h index 2a7ccea628b..5c76e1956c0 100644 --- a/packages/react-native/ReactCommon/react/runtime/ReactInstance.h +++ b/packages/react-native/ReactCommon/react/runtime/ReactInstance.h @@ -20,12 +20,6 @@ namespace facebook::react { -struct CallableModule { - explicit CallableModule(jsi::Function factory) - : factory(std::move(factory)) {} - jsi::Function factory; -}; - class ReactInstance final : private jsinspector_modern::InstanceTargetDelegate { public: using BindingsInstallFunc = std::function; @@ -61,7 +55,7 @@ class ReactInstance final : private jsinspector_modern::InstanceTargetDelegate { void callFunctionOnModule( const std::string& moduleName, const std::string& methodName, - const folly::dynamic& args); + folly::dynamic&& args); void handleMemoryPressureJs(int pressureLevel); @@ -78,7 +72,8 @@ class ReactInstance final : private jsinspector_modern::InstanceTargetDelegate { std::shared_ptr jsMessageQueueThread_; std::shared_ptr bufferedRuntimeExecutor_; std::shared_ptr timerManager_; - std::unordered_map> modules_; + std::unordered_map> + callableModules_; std::shared_ptr runtimeScheduler_; std::shared_ptr jsErrorHandler_; diff --git a/packages/react-native/ReactCommon/react/runtime/tests/cxx/ReactInstanceTest.cpp b/packages/react-native/ReactCommon/react/runtime/tests/cxx/ReactInstanceTest.cpp index 9ba76917999..b69d831aade 100644 --- a/packages/react-native/ReactCommon/react/runtime/tests/cxx/ReactInstanceTest.cpp +++ b/packages/react-native/ReactCommon/react/runtime/tests/cxx/ReactInstanceTest.cpp @@ -18,6 +18,7 @@ #include using ::testing::_; +using ::testing::HasSubstr; using ::testing::SaveArg; namespace facebook::react { @@ -801,9 +802,10 @@ TEST_F(ReactInstanceTest, testCallFunctionOnModule_invalidModule) { instance_->callFunctionOnModule("invalidModule", "method", std::move(args)); step(); expectError(); - EXPECT_EQ( + EXPECT_THAT( getLastErrorMessage(), - "Failed to call into JavaScript module method invalidModule.method(). Module has not been registered as callable. Registered callable JavaScript modules (n = 0):. Did you forget to call `RN$registerCallableModule`?"); + HasSubstr( + "Failed to call into JavaScript module method invalidModule.method()")); } TEST_F(ReactInstanceTest, testCallFunctionOnModule_undefinedMethod) { @@ -820,7 +822,7 @@ RN$registerCallableModule('foo', () => module); expectError(); EXPECT_EQ( getLastErrorMessage(), - "Failed to call into JavaScript module method foo.invalidMethod. Module exists, but the method is undefined."); + "getPropertyAsObject: property 'invalidMethod' is undefined, expected an Object"); } TEST_F(ReactInstanceTest, testCallFunctionOnModule_invalidMethod) {