From 13e70d7bd694b7c310f6278b8d04100f8fe333a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kr=C3=A6n=20Hansen?= Date: Thu, 18 Jul 2024 06:24:52 +0200 Subject: [PATCH] RJS-2852: Adding a `CallInvoker`-based scheduler for Core on React Native (#6791) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Removing the FlushMicrotaskQueueGuard and flush_ui_workaround * Adding a ReactScheduler * Rename injectCallInvoker → createScheduler * Updating comments * Ran formatting * Incorporated feedback and clarified the need for the shared_ptr * Make watchman ignore .wireit dirs * Updated package.lock * Updating comments * Adding a note to the changelog * Simplified capture for lambda passed to invokeAsync --- .watchmanconfig | 4 +- CHANGELOG.md | 5 +- .../react-native-test-app/ios/Podfile.lock | 2 +- .../realm/bindgen/src/realm_js_jsi_helpers.h | 9 -- packages/realm/bindgen/src/templates/jsi.ts | 2 - packages/realm/binding/android/CMakeLists.txt | 2 +- .../cpp/io_realm_react_RealmReactModule.cpp | 19 ++-- .../java/io/realm/react/RealmReactModule.java | 8 +- .../realm/binding/apple/RealmReactModule.mm | 13 +-- .../binding/jsi/flush_ui_queue_workaround.cpp | 62 ------------ .../realm/binding/jsi/react_scheduler.cpp | 98 +++++++++++++++++++ ...i_queue_workaround.h => react_scheduler.h} | 9 +- 12 files changed, 129 insertions(+), 104 deletions(-) delete mode 100644 packages/realm/binding/jsi/flush_ui_queue_workaround.cpp create mode 100644 packages/realm/binding/jsi/react_scheduler.cpp rename packages/realm/binding/jsi/{flush_ui_queue_workaround.h => react_scheduler.h} (77%) diff --git a/.watchmanconfig b/.watchmanconfig index 4a519b118a..a29cb1875f 100644 --- a/.watchmanconfig +++ b/.watchmanconfig @@ -3,6 +3,8 @@ "react-native/node_modules", "packages/realm-web", "packages/realm-web-integration-tests", - ".wireit" + ".wireit", + "packages/realm/.wireit", + "integration-tests/environments/react-native-test-app/.wireit" ] } diff --git a/CHANGELOG.md b/CHANGELOG.md index bc9643464e..585e7f3c81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,9 +24,8 @@ realm.syncSession?.addProgressNotification( * File format: generates Realms with format v24 (reads and upgrades file format v10). ### Internal - - - +* Adding a CallInvoker-based scheduler for Core on React Native and removing the "flush ui queue" workaround. ([#6791](https://github.com/realm/realm-js/pull/6791)) +* Refactors throwing uncaught exceptions from callbacks dispatched onto the event loop from C++ on React Native. ([#6772](https://github.com/realm/realm-js/issues/6772)) ## 12.11.1 (2024-06-25) diff --git a/integration-tests/environments/react-native-test-app/ios/Podfile.lock b/integration-tests/environments/react-native-test-app/ios/Podfile.lock index 3f1ec42c0a..0139d93405 100644 --- a/integration-tests/environments/react-native-test-app/ios/Podfile.lock +++ b/integration-tests/environments/react-native-test-app/ios/Podfile.lock @@ -1449,7 +1449,7 @@ SPEC CHECKSUMS: ReactNativeHost: fd9d65f6ee7947f468537d35b3fefe8b2bf546da ReactTestApp-DevSupport: 33a7ae9bf22161f359629a9607f5ef27e9c91fd0 ReactTestApp-Resources: d200e68756fa45c648f369210bd7ee0c14759f5a - RealmJS: b7ed9bcfaac0946479403b684d158d922f41384e + RealmJS: 298cc4766e917bd415fe0cfbfd1f4b198ab812f4 RNFS: 4ac0f0ea233904cb798630b3c077808c06931688 SocketRocket: abac6f5de4d4d62d24e11868d7a2f427e0ef940d Yoga: ae3c32c514802d30f687a04a6a35b348506d411f diff --git a/packages/realm/bindgen/src/realm_js_jsi_helpers.h b/packages/realm/bindgen/src/realm_js_jsi_helpers.h index a107bdf55c..275a2c9137 100644 --- a/packages/realm/bindgen/src/realm_js_jsi_helpers.h +++ b/packages/realm/bindgen/src/realm_js_jsi_helpers.h @@ -4,18 +4,9 @@ #include #include -#include - namespace realm::js::JSI { namespace { -struct FlushMicrotaskQueueGuard { - ~FlushMicrotaskQueueGuard() - { - realm::js::flush_ui_workaround::flush_ui_queue(); - } -}; - namespace jsi = facebook::jsi; template struct HostRefWrapper : jsi::HostObject { diff --git a/packages/realm/bindgen/src/templates/jsi.ts b/packages/realm/bindgen/src/templates/jsi.ts index c0f462ff18..3b37eba700 100644 --- a/packages/realm/bindgen/src/templates/jsi.ts +++ b/packages/realm/bindgen/src/templates/jsi.ts @@ -652,8 +652,6 @@ function convertFromJsi(addon: JsiAddon, type: Type, expr: string): string { { _thread.assertOnSameThread(); auto& _env = ${addon.get()}->m_rt; - // TODO consider not flushing when calling back into JS from withing a JS->CPP call. - FlushMicrotaskQueueGuard guard; return ${c( type.ret, `_cb->call( diff --git a/packages/realm/binding/android/CMakeLists.txt b/packages/realm/binding/android/CMakeLists.txt index c64f2660f6..e637afec0d 100644 --- a/packages/realm/binding/android/CMakeLists.txt +++ b/packages/realm/binding/android/CMakeLists.txt @@ -13,7 +13,7 @@ endif() add_library(realm-js-android-binding SHARED ../jsi/jsi_init.cpp - ../jsi/flush_ui_queue_workaround.cpp + ../jsi/react_scheduler.cpp src/main/cpp/platform.cpp src/main/cpp/jni_utils.cpp src/main/cpp/io_realm_react_RealmReactModule.cpp diff --git a/packages/realm/binding/android/src/main/cpp/io_realm_react_RealmReactModule.cpp b/packages/realm/binding/android/src/main/cpp/io_realm_react_RealmReactModule.cpp index 71a3b31aab..a0220a61c9 100644 --- a/packages/realm/binding/android/src/main/cpp/io_realm_react_RealmReactModule.cpp +++ b/packages/realm/binding/android/src/main/cpp/io_realm_react_RealmReactModule.cpp @@ -24,7 +24,7 @@ #include #include "jsi_init.h" -#include "flush_ui_queue_workaround.h" +#include "react_scheduler.h" #include "platform.hpp" #include "jni_utils.hpp" @@ -108,8 +108,8 @@ extern "C" JNIEXPORT void JNICALL Java_io_realm_react_RealmReactModule_setDefaul realm::JsPlatformHelpers::default_realm_file_directory().c_str()); } -extern "C" JNIEXPORT void JNICALL Java_io_realm_react_RealmReactModule_injectCallInvoker(JNIEnv* env, jobject thiz, - jobject call_invoker) +extern "C" JNIEXPORT void JNICALL Java_io_realm_react_RealmReactModule_createScheduler(JNIEnv* env, jobject thiz, + jobject call_invoker) { // React Native uses the fbjni library for handling JNI, which has the concept of "hybrid objects", // which are Java objects containing a pointer to a C++ object. The CallInvokerHolder, which has the @@ -119,6 +119,9 @@ extern "C" JNIEXPORT void JNICALL Java_io_realm_react_RealmReactModule_injectCal // 1. Get the Java object referred to by the mHybridData field of the Java holder object auto callInvokerHolderClass = env->FindClass("com/facebook/react/turbomodule/core/CallInvokerHolderImpl"); + if (!env->IsInstanceOf(call_invoker, callInvokerHolderClass)) { + throw std::invalid_argument("Expected call_invoker to be CallInvokerHolderImpl"); + } auto hybridDataField = env->GetFieldID(callInvokerHolderClass, "mHybridData", "Lcom/facebook/jni/HybridData;"); auto hybridDataObj = env->GetObjectField(call_invoker, hybridDataField); @@ -136,14 +139,16 @@ extern "C" JNIEXPORT void JNICALL Java_io_realm_react_RealmReactModule_injectCal // 4. Cast the mNativePointer back to its C++ type auto nativePointer = reinterpret_cast(nativePointerValue); - // 5. Inject the JS call invoker for the workaround to use - realm::js::flush_ui_workaround::inject_js_call_invoker(nativePointer->getCallInvoker()); + // 5. Create the scheduler from the JS call invoker + __android_log_print(ANDROID_LOG_VERBOSE, "Realm", "Creating scheduler"); + realm::js::react_scheduler::create_scheduler(nativePointer->getCallInvoker()); } extern "C" JNIEXPORT void JNICALL Java_io_realm_react_RealmReactModule_invalidateCaches(JNIEnv* env, jobject thiz) { - // Disable the flush ui workaround - realm::js::flush_ui_workaround::reset_js_call_invoker(); + __android_log_print(ANDROID_LOG_VERBOSE, "Realm", "Resetting scheduler"); + // Reset the scheduler to prevent invocations using an old runtime + realm::js::react_scheduler::reset_scheduler(); __android_log_print(ANDROID_LOG_VERBOSE, "Realm", "Invalidating caches"); #if DEBUG // Immediately close any open sync sessions to prevent race condition with new diff --git a/packages/realm/binding/android/src/main/java/io/realm/react/RealmReactModule.java b/packages/realm/binding/android/src/main/java/io/realm/react/RealmReactModule.java index 3f122083d6..7700e34405 100644 --- a/packages/realm/binding/android/src/main/java/io/realm/react/RealmReactModule.java +++ b/packages/realm/binding/android/src/main/java/io/realm/react/RealmReactModule.java @@ -77,7 +77,7 @@ public boolean injectModuleIntoJSGlobal() { } CallInvokerHolder jsCallInvokerHolder = reactContext.getCatalystInstance().getJSCallInvokerHolder(); - injectCallInvoker(jsCallInvokerHolder); + createScheduler(jsCallInvokerHolder); // Get the javascript runtime and inject our native module with it JavaScriptContextHolder jsContext = reactContext.getJavaScriptContextHolder(); @@ -103,11 +103,9 @@ public boolean injectModuleIntoJSGlobal() { private native void injectModuleIntoJSGlobal(long runtimePointer); /** - * Passes the React Native jsCallInvokerHolder over to C++ so we can setup our UI queue flushing. - * This is needed as a workaround for https://github.com/facebook/react-native/issues/33006 - * where we call the invokeAsync method to flush the React Native UI queue whenever we call from C++ to JS. + * Passes the React Native jsCallInvokerHolder over to C++ so we can setup a scheduler. */ - private native void injectCallInvoker(CallInvokerHolder callInvoker); + private native void createScheduler(CallInvokerHolder callInvoker); private native void invalidateCaches(); } diff --git a/packages/realm/binding/apple/RealmReactModule.mm b/packages/realm/binding/apple/RealmReactModule.mm index b68ecb1fa2..f2fe199c17 100644 --- a/packages/realm/binding/apple/RealmReactModule.mm +++ b/packages/realm/binding/apple/RealmReactModule.mm @@ -31,7 +31,7 @@ #import #import "jsi/jsi_init.h" -#import "flush_ui_queue_workaround.h" +#import "jsi/react_scheduler.h" // the part of the RCTCxxBridge private class we care about @interface RCTBridge (Realm_RCTCxxBridge) @@ -54,7 +54,8 @@ - (dispatch_queue_t)methodQueue { } - (void)invalidate { - realm::js::flush_ui_workaround::reset_js_call_invoker(); + // Reset the scheduler to prevent invocations using an old runtime + realm::js::react_scheduler::reset_scheduler(); #if DEBUG // Immediately close any open sync sessions to prevent race condition with new // JS thread when hot reloading @@ -74,12 +75,8 @@ - (void)dealloc { RCTBridge* bridge = [RCTBridge currentBridge]; auto &rt = *static_cast(bridge.runtime); - // Since https://github.com/facebook/react-native/pull/43396 this should only be needed when bridgeless is not enabled. - // but unfortunately, that doesn't seem to be the case. - // See https://github.com/facebook/react-native/pull/43396#issuecomment-2178586017 for context - // If it was, we could use the enablement of "microtasks" to avoid the overhead of calling the invokeAsync on every call from C++ into JS. - // if (!facebook::react::ReactNativeFeatureFlags::enableMicrotasks()) { - realm::js::flush_ui_workaround::inject_js_call_invoker([bridge jsCallInvoker]); + // Create a scheduler wrapping the call invoker and pass this to realm core + realm::js::react_scheduler::create_scheduler([bridge jsCallInvoker]); auto exports = jsi::Object(rt); realm_jsi_init(rt, exports); diff --git a/packages/realm/binding/jsi/flush_ui_queue_workaround.cpp b/packages/realm/binding/jsi/flush_ui_queue_workaround.cpp deleted file mode 100644 index 4ff1261f7c..0000000000 --- a/packages/realm/binding/jsi/flush_ui_queue_workaround.cpp +++ /dev/null @@ -1,62 +0,0 @@ -//////////////////////////////////////////////////////////////////////////// -// -// Copyright 2024 Realm Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// -//////////////////////////////////////////////////////////////////////////// - -#include "flush_ui_queue_workaround.h" - -#include - -namespace realm::js::flush_ui_workaround { -// Calls are debounced using the waiting_for_ui_flush bool, so if an async flush is already -// pending when another JS to C++ call happens, we don't call invokeAsync again. This works -// because the work is performed before the microtask queue is flushed - see sequence -// diagram at https://bit.ly/3kexhHm. It might be possible to further optimize this, -// e.g. only flush the queue a maximum of once per frame, but this seems reasonable. -bool waiting_for_ui_flush = false; -std::shared_ptr js_invoker_{}; - -void inject_js_call_invoker(std::shared_ptr js_invoker) -{ - js_invoker_ = js_invoker; -} - -void reset_js_call_invoker() -{ - // TODO: Consider taking an invoker as an argument and only resetting if it matches js_invoker_ - // This would ensure the pointer isn't resetting from a subsequent assignment from the constructor - js_invoker_.reset(); -} - -void flush_ui_queue() -{ - if (js_invoker_ && !waiting_for_ui_flush) { - waiting_for_ui_flush = true; - // Calling invokeAsync tells React Native to execute the lambda passed - // in on the JS thread, and then flush the internal "microtask queue", which has the - // effect of flushing any pending UI updates. - // - // We call this after we have called into JS from C++, in order to ensure that the RN - // UI updates in response to any changes from Realm. We need to do this as we bypass - // the usual RN bridge mechanism for communicating between C++ and JS, so without doing - // this RN has no way to know that a change has occurred which might require an update - // (see #4389, facebook/react-native#33006). - js_invoker_->invokeAsync([&] { - waiting_for_ui_flush = false; - }); - } -} -} // namespace realm::js::flush_ui_workaround diff --git a/packages/realm/binding/jsi/react_scheduler.cpp b/packages/realm/binding/jsi/react_scheduler.cpp new file mode 100644 index 0000000000..6b0e359f30 --- /dev/null +++ b/packages/realm/binding/jsi/react_scheduler.cpp @@ -0,0 +1,98 @@ +//////////////////////////////////////////////////////////////////////////// +// +// Copyright 2024 Realm Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +//////////////////////////////////////////////////////////////////////////// + +#include "react_scheduler.h" + +#include + +#include +#include + +#include + +using Scheduler = realm::util::Scheduler; + +namespace { + +std::shared_ptr scheduler_{}; + +class ReactScheduler : public realm::util::Scheduler { +public: + ReactScheduler(std::shared_ptr js_call_invoker) + : m_js_call_invoker(js_call_invoker) + { + } + + bool is_on_thread() const noexcept override + { + return m_id == std::this_thread::get_id(); + } + + bool is_same_as(const Scheduler* other) const noexcept override + { + auto o = dynamic_cast(other); + return (o && (o->m_js_call_invoker == m_js_call_invoker)); + } + + bool can_invoke() const noexcept override + { + return true; + } + + void invoke(realm::util::UniqueFunction&& func) override + { + m_js_call_invoker->invokeAsync( + // Using normal priority to avoid blocking rendering, user-input and other higher priority tasks + facebook::react::SchedulerPriority::NormalPriority, [ptr = func.release()] { + (realm::util::UniqueFunction(ptr))(); + }); + } + +private: + std::shared_ptr m_js_call_invoker; + std::thread::id m_id = std::this_thread::get_id(); +}; + +std::shared_ptr get_scheduler() +{ + if (scheduler_) { + REALM_ASSERT(scheduler_->is_on_thread()); + return scheduler_; + } + else { + return Scheduler::make_platform_default(); + } +} + +} // namespace + +namespace realm::js::react_scheduler { + + +void create_scheduler(std::shared_ptr js_call_invoker) +{ + scheduler_ = std::make_shared(js_call_invoker); + Scheduler::set_default_factory(get_scheduler); +} + +void reset_scheduler() +{ + scheduler_.reset(); +} + +} // namespace realm::js::react_scheduler diff --git a/packages/realm/binding/jsi/flush_ui_queue_workaround.h b/packages/realm/binding/jsi/react_scheduler.h similarity index 77% rename from packages/realm/binding/jsi/flush_ui_queue_workaround.h rename to packages/realm/binding/jsi/react_scheduler.h index 5889a58b44..4555bb57e6 100644 --- a/packages/realm/binding/jsi/flush_ui_queue_workaround.h +++ b/packages/realm/binding/jsi/react_scheduler.h @@ -20,8 +20,7 @@ #include -namespace realm::js::flush_ui_workaround { -void inject_js_call_invoker(std::shared_ptr js_invoker); -void reset_js_call_invoker(); -void flush_ui_queue(); -} // namespace realm::js::flush_ui_workaround +namespace realm::js::react_scheduler { +void create_scheduler(std::shared_ptr js_invoker); +void reset_scheduler(); +} // namespace realm::js::react_scheduler