Skip to content

Commit d7b37c4

Browse files
javachefacebook-github-bot
javache
authored and
facebook-github-bot
committed
Remove JsToNativeBridge's nativeQueue
Reviewed By: mhorowitz Differential Revision: D4589737 fbshipit-source-id: 3b2730417d99c4f98cfaad386bc50328f2551592
1 parent fbf6d1a commit d7b37c4

13 files changed

+143
-181
lines changed

‎React/CxxBridge/RCTCxxBridge.mm

+18-63
Original file line numberDiff line numberDiff line change
@@ -101,70 +101,25 @@ static bool isRAMBundle(NSData *script) {
101101
// is not the JS thread. C++ modules don't use RCTNativeModule, so this little
102102
// adapter does the work.
103103

104-
class QueueNativeModule : public NativeModule {
104+
class DispatchMessageQueueThread : public MessageQueueThread {
105105
public:
106-
QueueNativeModule(RCTCxxBridge *bridge,
107-
std::unique_ptr<NativeModule> module)
108-
: bridge_(bridge)
109-
, module_(std::move(module))
110-
// Create new queue (store queueName, as it isn't retained by dispatch_queue)
111-
, queueName_("com.facebook.react." + module_->getName() + "Queue")
112-
, queue_(dispatch_queue_create(queueName_.c_str(), DISPATCH_QUEUE_SERIAL)) {}
113-
114-
std::string getName() override {
115-
return module_->getName();
116-
}
117-
118-
std::vector<MethodDescriptor> getMethods() override {
119-
return module_->getMethods();
120-
}
121-
122-
folly::dynamic getConstants() override {
123-
return module_->getConstants();
124-
}
125-
126-
bool supportsWebWorkers() override {
127-
return module_->supportsWebWorkers();
128-
}
106+
DispatchMessageQueueThread(RCTModuleData *moduleData)
107+
: moduleData_(moduleData) {}
129108

130-
void invoke(ExecutorToken token, unsigned int reactMethodId,
131-
folly::dynamic &&params) override {
132-
__weak RCTCxxBridge *bridge = bridge_;
133-
auto module = module_;
134-
auto cparams = std::make_shared<folly::dynamic>(std::move(params));
135-
dispatch_async(queue_, ^{
136-
if (!bridge || !bridge.valid) {
137-
return;
138-
}
139-
140-
module->invoke(token, reactMethodId, std::move(*cparams));
109+
void runOnQueue(std::function<void()>&& func) override {
110+
dispatch_async(moduleData_.methodQueue, [func=std::move(func)] {
111+
func();
141112
});
142113
}
143-
144-
MethodCallResult callSerializableNativeHook(
145-
ExecutorToken token, unsigned int reactMethodId,
146-
folly::dynamic &&args) override {
147-
return module_->callSerializableNativeHook(token, reactMethodId, std::move(args));
114+
void runOnQueueSync(std::function<void()>&& func) override {
115+
LOG(FATAL) << "Unsupported operation";
116+
}
117+
void quitSynchronous() override {
118+
LOG(FATAL) << "Unsupported operation";
148119
}
149120

150121
private:
151-
RCTCxxBridge *bridge_;
152-
std::shared_ptr<NativeModule> module_;
153-
std::string queueName_;
154-
dispatch_queue_t queue_;
155-
};
156-
157-
158-
// This is a temporary hack. The cxx bridge assumes a single native
159-
// queue handles all native method calls, but that's false on ios. So
160-
// this is a no-thread passthrough, and queues are handled in
161-
// RCTNativeModule. A similar refactoring should be done on the Java
162-
// bridge.
163-
class InlineMessageQueueThread : public MessageQueueThread {
164-
public:
165-
void runOnQueue(std::function<void()>&& func) override { func(); }
166-
void runOnQueueSync(std::function<void()>&& func) override { func(); }
167-
void quitSynchronous() override {}
122+
RCTModuleData *moduleData_;
168123
};
169124

170125
}
@@ -551,12 +506,13 @@ - (BOOL)moduleIsInitialized:(Class)moduleClass
551506
if (![moduleData hasInstance]) {
552507
continue;
553508
}
554-
modules.emplace_back(
555-
new QueueNativeModule(self, std::make_unique<CxxNativeModule>(
556-
_reactInstance, [moduleData.name UTF8String],
557-
[moduleData] { return [(RCTCxxModule *)(moduleData.instance) move]; })));
509+
modules.emplace_back(std::make_unique<CxxNativeModule>(
510+
_reactInstance,
511+
[moduleData.name UTF8String],
512+
[moduleData] { return [(RCTCxxModule *)(moduleData.instance) move]; },
513+
std::make_shared<DispatchMessageQueueThread>(moduleData)));
558514
} else {
559-
modules.emplace_back(new RCTNativeModule(self, moduleData));
515+
modules.emplace_back(std::make_unique<RCTNativeModule>(self, moduleData));
560516
}
561517
}
562518

@@ -586,7 +542,6 @@ - (void)_initializeBridge:(std::shared_ptr<JSExecutorFactory>)executorFactory
586542
std::unique_ptr<RCTInstanceCallback>(new RCTInstanceCallback(self)),
587543
executorFactory,
588544
_jsMessageThread,
589-
std::unique_ptr<MessageQueueThread>(new InlineMessageQueueThread),
590545
std::move([self _createModuleRegistry]));
591546

592547
#if RCT_PROFILE

‎ReactAndroid/src/main/jni/xreact/jni/CatalystInstanceImpl.cpp

+26-16
Original file line numberDiff line numberDiff line change
@@ -41,13 +41,17 @@ class Exception : public jni::JavaClass<Exception> {
4141

4242
class JInstanceCallback : public InstanceCallback {
4343
public:
44-
explicit JInstanceCallback(alias_ref<ReactCallback::javaobject> jobj)
45-
: jobj_(make_global(jobj)) {}
44+
explicit JInstanceCallback(
45+
alias_ref<ReactCallback::javaobject> jobj,
46+
std::shared_ptr<JMessageQueueThread> messageQueueThread)
47+
: jobj_(make_global(jobj)), messageQueueThread_(std::move(messageQueueThread)) {}
4648

4749
void onBatchComplete() override {
48-
static auto method =
49-
ReactCallback::javaClassStatic()->getMethod<void()>("onBatchComplete");
50-
method(jobj_);
50+
messageQueueThread_->runOnQueue([this] {
51+
static auto method =
52+
ReactCallback::javaClassStatic()->getMethod<void()>("onBatchComplete");
53+
method(jobj_);
54+
});
5155
}
5256

5357
void incrementPendingJSCalls() override {
@@ -61,6 +65,7 @@ class JInstanceCallback : public InstanceCallback {
6165
}
6266

6367
void decrementPendingJSCalls() override {
68+
jni::ThreadScope guard;
6469
static auto method =
6570
ReactCallback::javaClassStatic()->getMethod<void()>("decrementPendingJSCalls");
6671
method(jobj_);
@@ -81,12 +86,11 @@ class JInstanceCallback : public InstanceCallback {
8186
return jobj->cthis()->getExecutorToken(jobj);
8287
}
8388

84-
void onExecutorStopped(ExecutorToken) override {
85-
// TODO(cjhopman): implement this.
86-
}
89+
void onExecutorStopped(ExecutorToken) override {}
8790

8891
private:
8992
global_ref<ReactCallback::javaobject> jobj_;
93+
std::shared_ptr<JMessageQueueThread> messageQueueThread_;
9094
};
9195

9296
}
@@ -97,7 +101,12 @@ jni::local_ref<CatalystInstanceImpl::jhybriddata> CatalystInstanceImpl::initHybr
97101
}
98102

99103
CatalystInstanceImpl::CatalystInstanceImpl()
100-
: instance_(folly::make_unique<Instance>()) {}
104+
: instance_(folly::make_unique<Instance>()) {}
105+
106+
CatalystInstanceImpl::~CatalystInstanceImpl() {
107+
// TODO: 16669252: this prevents onCatalystInstanceDestroy from being called
108+
moduleMessageQueue_->quitSynchronous();
109+
}
101110

102111
void CatalystInstanceImpl::registerNatives() {
103112
registerHybrid({
@@ -127,11 +136,12 @@ void CatalystInstanceImpl::initializeBridge(
127136
// This executor is actually a factory holder.
128137
JavaScriptExecutorHolder* jseh,
129138
jni::alias_ref<JavaMessageQueueThread::javaobject> jsQueue,
130-
jni::alias_ref<JavaMessageQueueThread::javaobject> moduleQueue,
139+
jni::alias_ref<JavaMessageQueueThread::javaobject> nativeModulesQueue,
131140
jni::alias_ref<jni::JCollection<JavaModuleWrapper::javaobject>::javaobject> javaModules,
132141
jni::alias_ref<jni::JCollection<ModuleHolder::javaobject>::javaobject> cxxModules) {
133142
// TODO mhorowitz: how to assert here?
134143
// Assertions.assertCondition(mBridge == null, "initializeBridge should be called once");
144+
moduleMessageQueue_ = std::make_shared<JMessageQueueThread>(nativeModulesQueue);
135145

136146
// This used to be:
137147
//
@@ -149,12 +159,12 @@ void CatalystInstanceImpl::initializeBridge(
149159
// don't need jsModuleDescriptions any more, all the way up and down the
150160
// stack.
151161

152-
instance_->initializeBridge(folly::make_unique<JInstanceCallback>(callback),
153-
jseh->getExecutorFactory(),
154-
folly::make_unique<JMessageQueueThread>(jsQueue),
155-
folly::make_unique<JMessageQueueThread>(moduleQueue),
156-
buildModuleRegistry(std::weak_ptr<Instance>(instance_),
157-
javaModules, cxxModules));
162+
instance_->initializeBridge(
163+
folly::make_unique<JInstanceCallback>(callback, moduleMessageQueue_),
164+
jseh->getExecutorFactory(),
165+
folly::make_unique<JMessageQueueThread>(jsQueue),
166+
buildModuleRegistry(
167+
std::weak_ptr<Instance>(instance_), javaModules, cxxModules, moduleMessageQueue_));
158168
}
159169

160170
void CatalystInstanceImpl::jniSetSourceURL(const std::string& sourceURL) {

‎ReactAndroid/src/main/jni/xreact/jni/CatalystInstanceImpl.h

+2
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ class CatalystInstanceImpl : public jni::HybridClass<CatalystInstanceImpl> {
2828
static constexpr auto kJavaDescriptor = "Lcom/facebook/react/cxxbridge/CatalystInstanceImpl;";
2929

3030
static jni::local_ref<jhybriddata> initHybrid(jni::alias_ref<jclass>);
31+
~CatalystInstanceImpl() override;
3132

3233
static void registerNatives();
3334

@@ -74,6 +75,7 @@ class CatalystInstanceImpl : public jni::HybridClass<CatalystInstanceImpl> {
7475
// This should be the only long-lived strong reference, but every C++ class
7576
// will have a weak reference.
7677
std::shared_ptr<Instance> instance_;
78+
std::shared_ptr<JMessageQueueThread> moduleMessageQueue_;
7779
};
7880

7981
}}

‎ReactAndroid/src/main/jni/xreact/jni/JavaModuleWrapper.cpp

+25-17
Original file line numberDiff line numberDiff line change
@@ -87,30 +87,36 @@ bool JavaNativeModule::supportsWebWorkers() {
8787
}
8888

8989
void JavaNativeModule::invoke(ExecutorToken token, unsigned int reactMethodId, folly::dynamic&& params) {
90-
static auto invokeMethod =
91-
wrapper_->getClass()->getMethod<void(JExecutorToken::javaobject, jint, ReadableNativeArray::javaobject)>("invoke");
92-
invokeMethod(wrapper_, JExecutorToken::extractJavaPartFromToken(token).get(), static_cast<jint>(reactMethodId),
93-
ReadableNativeArray::newObjectCxxArgs(std::move(params)).get());
90+
messageQueueThread_->runOnQueue([this, token, reactMethodId, params=std::move(params)] {
91+
static auto invokeMethod = wrapper_->getClass()->
92+
getMethod<void(JExecutorToken::javaobject, jint, ReadableNativeArray::javaobject)>("invoke");
93+
invokeMethod(wrapper_,
94+
JExecutorToken::extractJavaPartFromToken(token).get(),
95+
static_cast<jint>(reactMethodId),
96+
ReadableNativeArray::newObjectCxxArgs(std::move(params)).get());
97+
});
9498
}
9599

96100
MethodCallResult JavaNativeModule::callSerializableNativeHook(ExecutorToken token, unsigned int reactMethodId, folly::dynamic&& params) {
97-
// TODO: evaluate whether calling through invoke is potentially faster
98-
if (reactMethodId >= syncMethods_.size()) {
99-
throw std::invalid_argument(
100-
folly::to<std::string>("methodId ", reactMethodId, " out of range [0..", syncMethods_.size(), "]"));
101-
}
101+
// TODO: evaluate whether calling through invoke is potentially faster
102+
if (reactMethodId >= syncMethods_.size()) {
103+
throw std::invalid_argument(
104+
folly::to<std::string>("methodId ", reactMethodId, " out of range [0..", syncMethods_.size(), "]"));
105+
}
102106

103-
auto& method = syncMethods_[reactMethodId];
104-
CHECK(method.hasValue() && method->isSyncHook()) << "Trying to invoke a asynchronous method as synchronous hook";
105-
return method->invoke(instance_, wrapper_->getModule(), token, params);
107+
auto& method = syncMethods_[reactMethodId];
108+
CHECK(method.hasValue() && method->isSyncHook()) << "Trying to invoke a asynchronous method as synchronous hook";
109+
return method->invoke(instance_, wrapper_->getModule(), token, params);
106110
}
107111

108112
NewJavaNativeModule::NewJavaNativeModule(
109113
std::weak_ptr<Instance> instance,
110-
jni::alias_ref<JavaModuleWrapper::javaobject> wrapper)
111-
: instance_(std::move(instance)),
112-
wrapper_(make_global(wrapper)),
113-
module_(make_global(wrapper->getModule())) {
114+
jni::alias_ref<JavaModuleWrapper::javaobject> wrapper,
115+
std::shared_ptr<MessageQueueThread> messageQueueThread)
116+
: instance_(std::move(instance))
117+
, wrapper_(make_global(wrapper))
118+
, module_(make_global(wrapper->getModule()))
119+
, messageQueueThread_(std::move(messageQueueThread)) {
114120
auto descs = wrapper_->getMethodDescriptors();
115121
std::string moduleName = getName();
116122
methods_.reserve(descs->size());
@@ -161,7 +167,9 @@ void NewJavaNativeModule::invoke(ExecutorToken token, unsigned int reactMethodId
161167
folly::to<std::string>("methodId ", reactMethodId, " out of range [0..", methods_.size(), "]"));
162168
}
163169
CHECK(!methods_[reactMethodId].isSyncHook()) << "Trying to invoke a synchronous hook asynchronously";
164-
invokeInner(token, reactMethodId, std::move(params));
170+
messageQueueThread_->runOnQueue([this, token, reactMethodId, params=std::move(params)] () mutable {
171+
invokeInner(token, reactMethodId, std::move(params));
172+
});
165173
}
166174

167175
MethodCallResult NewJavaNativeModule::callSerializableNativeHook(ExecutorToken token, unsigned int reactMethodId, folly::dynamic&& params) {

‎ReactAndroid/src/main/jni/xreact/jni/JavaModuleWrapper.h

+10-3
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ namespace facebook {
1212
namespace react {
1313

1414
class Instance;
15+
class MessageQueueThread;
1516

1617
struct JMethodDescriptor : public jni::JavaClass<JMethodDescriptor> {
1718
static constexpr auto kJavaDescriptor =
@@ -44,8 +45,11 @@ class JavaNativeModule : public NativeModule {
4445
public:
4546
JavaNativeModule(
4647
std::weak_ptr<Instance> instance,
47-
jni::alias_ref<JavaModuleWrapper::javaobject> wrapper)
48-
: instance_(std::move(instance)), wrapper_(make_global(wrapper)) {}
48+
jni::alias_ref<JavaModuleWrapper::javaobject> wrapper,
49+
std::shared_ptr<MessageQueueThread> messageQueueThread)
50+
: instance_(std::move(instance))
51+
, wrapper_(make_global(wrapper))
52+
, messageQueueThread_(std::move(messageQueueThread)) {}
4953

5054
std::string getName() override;
5155
folly::dynamic getConstants() override;
@@ -57,6 +61,7 @@ class JavaNativeModule : public NativeModule {
5761
private:
5862
std::weak_ptr<Instance> instance_;
5963
jni::global_ref<JavaModuleWrapper::javaobject> wrapper_;
64+
std::shared_ptr<MessageQueueThread> messageQueueThread_;
6065
std::vector<folly::Optional<MethodInvoker>> syncMethods_;
6166
};
6267

@@ -65,7 +70,8 @@ class NewJavaNativeModule : public NativeModule {
6570
public:
6671
NewJavaNativeModule(
6772
std::weak_ptr<Instance> instance,
68-
jni::alias_ref<JavaModuleWrapper::javaobject> wrapper);
73+
jni::alias_ref<JavaModuleWrapper::javaobject> wrapper,
74+
std::shared_ptr<MessageQueueThread> messageQueueThread);
6975

7076
std::string getName() override;
7177
std::vector<MethodDescriptor> getMethods() override;
@@ -78,6 +84,7 @@ class NewJavaNativeModule : public NativeModule {
7884
std::weak_ptr<Instance> instance_;
7985
jni::global_ref<JavaModuleWrapper::javaobject> wrapper_;
8086
jni::global_ref<JBaseJavaModule::javaobject> module_;
87+
std::shared_ptr<MessageQueueThread> messageQueueThread_;
8188
std::vector<MethodInvoker> methods_;
8289
std::vector<MethodDescriptor> methodDescriptors_;
8390

‎ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryBuilder.cpp

+6-4
Original file line numberDiff line numberDiff line change
@@ -32,14 +32,16 @@ xplat::module::CxxModule::Provider ModuleHolder::getProvider() const {
3232
std::unique_ptr<ModuleRegistry> buildModuleRegistry(
3333
std::weak_ptr<Instance> winstance,
3434
jni::alias_ref<jni::JCollection<JavaModuleWrapper::javaobject>::javaobject> javaModules,
35-
jni::alias_ref<jni::JCollection<ModuleHolder::javaobject>::javaobject> cxxModules) {
35+
jni::alias_ref<jni::JCollection<ModuleHolder::javaobject>::javaobject> cxxModules,
36+
std::shared_ptr<MessageQueueThread> moduleMessageQueue) {
3637
std::vector<std::unique_ptr<NativeModule>> modules;
3738
for (const auto& jm : *javaModules) {
38-
modules.emplace_back(folly::make_unique<JavaNativeModule>(winstance, jm));
39+
modules.emplace_back(folly::make_unique<JavaNativeModule>(
40+
winstance, jm, moduleMessageQueue));
3941
}
4042
for (const auto& cm : *cxxModules) {
41-
modules.emplace_back(
42-
folly::make_unique<CxxNativeModule>(winstance, cm->getName(), cm->getProvider()));
43+
modules.emplace_back(folly::make_unique<CxxNativeModule>(
44+
winstance, cm->getName(), cm->getProvider(), moduleMessageQueue));
4345
}
4446
if (modules.empty()) {
4547
return nullptr;

‎ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryBuilder.h

+4-1
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@
1212
namespace facebook {
1313
namespace react {
1414

15+
class MessageQueueThread;
16+
1517
class ModuleHolder : public jni::JavaClass<ModuleHolder> {
1618
public:
1719
static auto constexpr kJavaDescriptor =
@@ -24,7 +26,8 @@ class ModuleHolder : public jni::JavaClass<ModuleHolder> {
2426
std::unique_ptr<ModuleRegistry> buildModuleRegistry(
2527
std::weak_ptr<Instance> winstance,
2628
jni::alias_ref<jni::JCollection<JavaModuleWrapper::javaobject>::javaobject> javaModules,
27-
jni::alias_ref<jni::JCollection<ModuleHolder::javaobject>::javaobject> cxxModules);
29+
jni::alias_ref<jni::JCollection<ModuleHolder::javaobject>::javaobject> cxxModules,
30+
std::shared_ptr<MessageQueueThread> moduleMessageQueue);
2831

2932
}
3033
}

0 commit comments

Comments
 (0)