From 7176c5291e57ccbcbeab962359bb2063ee43dace Mon Sep 17 00:00:00 2001 From: Andy Street Date: Tue, 23 Feb 2016 10:57:49 -0800 Subject: [PATCH] Dispatch JS calls to JS thread from c++ bridge Summary: Instead of dispatching calls to the JS thread in Java, do it in the C++ bridge. This moves us closer to the cxx bridge and will allow us to dispatch to the correct web worker in C++ instead of in Java Reviewed By: mhorowitz Differential Revision: D2954115 fb-gh-sync-id: 7e7d4eff2c72601b8b4416f1ccd8d2985aebd755 shipit-source-id: 7e7d4eff2c72601b8b4416f1ccd8d2985aebd755 --- .../testing/ReactIntegrationTestCase.java | 3 + .../react/bridge/CatalystInstanceImpl.java | 114 +++++------------- .../facebook/react/bridge/ReactBridge.java | 2 +- ReactAndroid/src/main/jni/react/Bridge.cpp | 56 ++++++++- ReactAndroid/src/main/jni/react/Bridge.h | 15 ++- .../src/main/jni/react/jni/OnLoad.cpp | 5 +- 6 files changed, 103 insertions(+), 92 deletions(-) diff --git a/ReactAndroid/src/androidTest/java/com/facebook/react/testing/ReactIntegrationTestCase.java b/ReactAndroid/src/androidTest/java/com/facebook/react/testing/ReactIntegrationTestCase.java index e1312d36a6..b8d85fe0ff 100644 --- a/ReactAndroid/src/androidTest/java/com/facebook/react/testing/ReactIntegrationTestCase.java +++ b/ReactAndroid/src/androidTest/java/com/facebook/react/testing/ReactIntegrationTestCase.java @@ -70,14 +70,17 @@ public abstract class ReactIntegrationTestCase extends AndroidTestCase { mReactContext = null; mInstance = null; + final SimpleSettableFuture semaphore = new SimpleSettableFuture<>(); UiThreadUtil.runOnUiThread(new Runnable() { @Override public void run() { if (contextToDestroy != null) { contextToDestroy.destroy(); } + semaphore.set(null); } }); + semaphore.getOrThrow(); } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java index 325a123065..cbf259f29a 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java @@ -53,7 +53,7 @@ public class CatalystInstanceImpl implements CatalystInstance { private final TraceListener mTraceListener; private final JavaScriptModuleRegistry mJSModuleRegistry; private final JSBundleLoader mJSBundleLoader; - private volatile int mTraceID = 0; + private final Object mTeardownLock = new Object(); // Access from native modules thread private final NativeModuleRegistry mJavaRegistry; @@ -168,42 +168,16 @@ public class CatalystInstanceImpl implements CatalystInstance { final int methodId, final NativeArray arguments, final String tracingName) { - if (mDestroyed) { - FLog.w(ReactConstants.TAG, "Calling JS function after bridge has been destroyed."); - return; + synchronized (mTeardownLock) { + if (mDestroyed) { + FLog.w(ReactConstants.TAG, "Calling JS function after bridge has been destroyed."); + return; + } + + incrementPendingJSCalls(); + + Assertions.assertNotNull(mBridge).callFunction(moduleId, methodId, arguments, tracingName); } - - incrementPendingJSCalls(); - - final int traceID = mTraceID++; - Systrace.startAsyncFlow( - Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, - tracingName, - traceID); - - mReactQueueConfiguration.getJSQueueThread().runOnQueue( - new Runnable() { - @Override - public void run() { - mReactQueueConfiguration.getJSQueueThread().assertIsOnThread(); - - Systrace.endAsyncFlow( - Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, - tracingName, - traceID); - - if (mDestroyed) { - return; - } - - Systrace.beginSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, tracingName); - try { - Assertions.assertNotNull(mBridge).callFunction(moduleId, methodId, arguments); - } finally { - Systrace.endSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE); - } - } - }); } // This is called from java code, so it won't be stripped anyway, but proguard will rename it, @@ -211,42 +185,16 @@ public class CatalystInstanceImpl implements CatalystInstance { @DoNotStrip @Override public void invokeCallback(final int callbackID, final NativeArray arguments) { - if (mDestroyed) { - FLog.w(ReactConstants.TAG, "Invoking JS callback after bridge has been destroyed."); - return; + synchronized (mTeardownLock) { + if (mDestroyed) { + FLog.w(ReactConstants.TAG, "Invoking JS callback after bridge has been destroyed."); + return; + } + + incrementPendingJSCalls(); + + Assertions.assertNotNull(mBridge).invokeCallback(callbackID, arguments); } - - incrementPendingJSCalls(); - - final int traceID = mTraceID++; - Systrace.startAsyncFlow( - Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, - "", - traceID); - - mReactQueueConfiguration.getJSQueueThread().runOnQueue( - new Runnable() { - @Override - public void run() { - mReactQueueConfiguration.getJSQueueThread().assertIsOnThread(); - - Systrace.endAsyncFlow( - Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, - "", - traceID); - - if (mDestroyed) { - return; - } - - Systrace.beginSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, ""); - try { - Assertions.assertNotNull(mBridge).invokeCallback(callbackID, arguments); - } finally { - Systrace.endSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE); - } - } - }); } /** @@ -258,18 +206,20 @@ public class CatalystInstanceImpl implements CatalystInstance { public void destroy() { UiThreadUtil.assertOnUiThread(); - if (mDestroyed) { - return; + synchronized (mTeardownLock) { + if (mDestroyed) { + return; + } + + // TODO: tell all APIs to shut down + mDestroyed = true; + mJavaRegistry.notifyCatalystInstanceDestroy(); + + Systrace.unregisterListener(mTraceListener); + + synchronouslyDisposeBridgeOnJSThread(); + mReactQueueConfiguration.destroy(); } - - // TODO: tell all APIs to shut down - mDestroyed = true; - mJavaRegistry.notifyCatalystInstanceDestroy(); - - Systrace.unregisterListener(mTraceListener); - - synchronouslyDisposeBridgeOnJSThread(); - mReactQueueConfiguration.destroy(); boolean wasIdle = (mPendingJSCalls.getAndSet(0) == 0); if (!wasIdle && !mBridgeIdleListeners.isEmpty()) { for (NotThreadSafeBridgeIdleDebugListener listener : mBridgeIdleListeners) { diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactBridge.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactBridge.java index c088de3a6b..89b6870bec 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactBridge.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactBridge.java @@ -79,7 +79,7 @@ public class ReactBridge extends Countable { */ public native void loadScriptFromAssets(AssetManager assetManager, String assetName); public native void loadScriptFromFile(@Nullable String fileName, @Nullable String sourceURL); - public native void callFunction(int moduleId, int methodId, NativeArray arguments); + public native void callFunction(int moduleId, int methodId, NativeArray arguments, String tracingName); public native void invokeCallback(int callbackID, NativeArray arguments); public native void setGlobalVariable(String propertyName, String jsonEncodedArgument); public native boolean supportsProfiling(); diff --git a/ReactAndroid/src/main/jni/react/Bridge.cpp b/ReactAndroid/src/main/jni/react/Bridge.cpp index fe835fa51f..1c1a2bf34d 100644 --- a/ReactAndroid/src/main/jni/react/Bridge.cpp +++ b/ReactAndroid/src/main/jni/react/Bridge.cpp @@ -5,14 +5,18 @@ #ifdef WITH_FBSYSTRACE #include using fbsystrace::FbSystraceSection; +using fbsystrace::FbSystraceAsyncFlow; #endif +#include "Platform.h" + namespace facebook { namespace react { Bridge::Bridge(JSExecutorFactory* jsExecutorFactory, Callback callback) : m_callback(std::move(callback)), - m_destroyed(std::shared_ptr(new bool(false))) { + m_destroyed(std::make_shared(false)), + m_mainJSMessageQueueThread(MessageQueues::getCurrentMessageQueueThread()) { m_mainExecutor = jsExecutorFactory->createJSExecutor(this); } @@ -33,24 +37,64 @@ void Bridge::loadApplicationUnbundle( m_mainExecutor->loadApplicationUnbundle(std::move(unbundle), startupCode, sourceURL); } -void Bridge::callFunction(const double moduleId, const double methodId, const folly::dynamic& arguments) { +void Bridge::callFunction( + const double moduleId, + const double methodId, + const folly::dynamic& arguments, + const std::string& tracingName) { if (*m_destroyed) { return; } + #ifdef WITH_FBSYSTRACE - FbSystraceSection s(TRACE_TAG_REACT_CXX_BRIDGE, "Bridge.callFunction"); + int systraceCookie = m_systraceCookie++; + FbSystraceAsyncFlow::begin( + TRACE_TAG_REACT_CXX_BRIDGE, + tracingName.c_str(), + systraceCookie); #endif - m_mainExecutor->callFunction(moduleId, methodId, arguments); + std::shared_ptr isDestroyed = m_destroyed; + m_mainJSMessageQueueThread->runOnQueue([=] () { + if (*isDestroyed) { + return; + } + #ifdef WITH_FBSYSTRACE + FbSystraceAsyncFlow::end( + TRACE_TAG_REACT_CXX_BRIDGE, + tracingName.c_str(), + systraceCookie); + FbSystraceSection s(TRACE_TAG_REACT_CXX_BRIDGE, tracingName.c_str()); + #endif + m_mainExecutor->callFunction(moduleId, methodId, arguments); + }); } void Bridge::invokeCallback(const double callbackId, const folly::dynamic& arguments) { if (*m_destroyed) { return; } + #ifdef WITH_FBSYSTRACE - FbSystraceSection s(TRACE_TAG_REACT_CXX_BRIDGE, "Bridge.invokeCallback"); + int systraceCookie = m_systraceCookie++; + FbSystraceAsyncFlow::begin( + TRACE_TAG_REACT_CXX_BRIDGE, + "", + systraceCookie); #endif - m_mainExecutor->invokeCallback(callbackId, arguments); + std::shared_ptr isDestroyed = m_destroyed; + m_mainJSMessageQueueThread->runOnQueue([=] () { + if (*isDestroyed) { + return; + } + #ifdef WITH_FBSYSTRACE + FbSystraceAsyncFlow::end( + TRACE_TAG_REACT_CXX_BRIDGE, + "", + systraceCookie); + FbSystraceSection s(TRACE_TAG_REACT_CXX_BRIDGE, "Bridge.invokeCallback"); + #endif + m_mainExecutor->invokeCallback(callbackId, arguments); + }); } void Bridge::setGlobalVariable(const std::string& propName, const std::string& jsonValue) { diff --git a/ReactAndroid/src/main/jni/react/Bridge.h b/ReactAndroid/src/main/jni/react/Bridge.h index 1833ccba4e..3868d507ff 100644 --- a/ReactAndroid/src/main/jni/react/Bridge.h +++ b/ReactAndroid/src/main/jni/react/Bridge.h @@ -2,11 +2,13 @@ #pragma once +#include #include #include #include #include "Executor.h" +#include "MessageQueueThread.h" #include "MethodCall.h" #include "JSModulesUnbundle.h" #include "Value.h" @@ -24,6 +26,9 @@ class Bridge { public: typedef std::function, bool isEndOfBatch)> Callback; + /** + * This must be called on the main JS thread. + */ Bridge(JSExecutorFactory* jsExecutorFactory, Callback callback); virtual ~Bridge(); @@ -31,7 +36,11 @@ public: * Executes a function with the module ID and method ID and any additional * arguments in JS. */ - void callFunction(const double moduleId, const double methodId, const folly::dynamic& args); + void callFunction( + const double moduleId, + const double methodId, + const folly::dynamic& args, + const std::string& tracingName); /** * Invokes a callback with the cbID, and optional additional arguments in JS. @@ -72,6 +81,10 @@ private: // will have been destroyed within ~Bridge(), thus causing a SIGSEGV. std::shared_ptr m_destroyed; std::unique_ptr m_mainExecutor; + std::shared_ptr m_mainJSMessageQueueThread; + #ifdef WITH_FBSYSTRACE + std::atomic_uint_least32_t m_systraceCookie = ATOMIC_VAR_INIT(); + #endif }; } } diff --git a/ReactAndroid/src/main/jni/react/jni/OnLoad.cpp b/ReactAndroid/src/main/jni/react/jni/OnLoad.cpp index 1e4c379503..1a4dbe3c17 100644 --- a/ReactAndroid/src/main/jni/react/jni/OnLoad.cpp +++ b/ReactAndroid/src/main/jni/react/jni/OnLoad.cpp @@ -727,14 +727,15 @@ static void loadScriptFromFile(JNIEnv* env, jobject obj, jstring fileName, jstri } static void callFunction(JNIEnv* env, jobject obj, jint moduleId, jint methodId, - NativeArray::jhybridobject args) { + NativeArray::jhybridobject args, jstring tracingName) { auto bridge = extractRefPtr(env, obj); auto arguments = cthis(wrap_alias(args)); try { bridge->callFunction( (double) moduleId, (double) methodId, - std::move(arguments->array) + std::move(arguments->array), + fromJString(env, tracingName) ); } catch (...) { translatePendingCppExceptionToJavaException();