From 9883da167be2d5f4e594daac63baf20cab99b164 Mon Sep 17 00:00:00 2001 From: Valentin Shergin Date: Thu, 7 Nov 2019 09:41:11 -0800 Subject: [PATCH] Fabric: Removing RuntimeExecutor from EventBeatManager Summary: This diff changes the implementation a little bit: * We don't use RuntimeExecutor there anymore (because it's already been used in EventBeat). The actual purpose of EventBeatManager was distilled to "provide a tick synchronized with the main run loop"; * Now we use dedicated EventBeatManagerObserver interface to decouple this functionality from EventBeat-intrinsic functionality; * A bunch of small clean-ups. * Now we ensure threading and capturing ownership in a single function in AsyncEventBeat class. Before this change, the EventBeatManager called `beat` function of the EventBeat directly bypassing AsyncEventBeat class, and effectively bypassing the check that underlying infra still exists (aka EventBeat::OwnerBox feature). That alone might fix some C++ crashes that we see. Changelog: [Internal] Fabric-specific internal change. Reviewed By: sammy-SC Differential Revision: D18307655 fbshipit-source-id: 3b582cb71085ed99ee94f8e6d575196c2082557b --- .../react/fabric/jni/AsyncEventBeat.h | 40 +++++++++---------- .../com/facebook/react/fabric/jni/Binding.cpp | 2 - .../react/fabric/jni/EventBeatManager.cpp | 24 +++++------ .../react/fabric/jni/EventBeatManager.h | 33 ++++++++++----- 4 files changed, 50 insertions(+), 49 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/AsyncEventBeat.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/AsyncEventBeat.h index d90713e4dd0..cf09e4d5a69 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/AsyncEventBeat.h +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/AsyncEventBeat.h @@ -16,34 +16,25 @@ namespace facebook { namespace react { -namespace { - -class AsyncEventBeat : public EventBeat { - private: - EventBeatManager* eventBeatManager_; - RuntimeExecutor runtimeExecutor_; - jni::global_ref javaUIManager_; - +class AsyncEventBeat final : public EventBeat, public EventBeatManagerObserver { public: - friend class EventBeatManager; - AsyncEventBeat( EventBeat::SharedOwnerBox const &ownerBox, - EventBeatManager* eventBeatManager, + EventBeatManager *eventBeatManager, RuntimeExecutor runtimeExecutor, - jni::global_ref javaUIManager) : - EventBeat(ownerBox), - eventBeatManager_(eventBeatManager), - runtimeExecutor_(std::move(runtimeExecutor)), - javaUIManager_(javaUIManager) { - eventBeatManager->registerEventBeat(this); + jni::global_ref javaUIManager) + : EventBeat(ownerBox), + eventBeatManager_(eventBeatManager), + runtimeExecutor_(runtimeExecutor), + javaUIManager_(javaUIManager) { + eventBeatManager->addObserver(*this); } ~AsyncEventBeat() { - eventBeatManager_->unregisterEventBeat(this); + eventBeatManager_->removeObserver(*this); } - void induce() const override { + void tick() const override { runtimeExecutor_([this, ownerBox = ownerBox_](jsi::Runtime &runtime) { auto owner = ownerBox->owner.lock(); if (!owner) { @@ -54,6 +45,10 @@ class AsyncEventBeat : public EventBeat { }); } + void induce() const override { + tick(); + } + void request() const override { bool alreadyRequested = isRequested_; EventBeat::request(); @@ -65,9 +60,12 @@ class AsyncEventBeat : public EventBeat { onRequestEventBeat(javaUIManager_); } } -}; -} // namespace + private: + EventBeatManager *eventBeatManager_; + RuntimeExecutor runtimeExecutor_; + jni::global_ref javaUIManager_; +}; } // namespace react } // namespace facebook diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp index b6638a439a8..4dc1556f05a 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp @@ -214,8 +214,6 @@ void Binding::installFabricUIManager( }); }; - eventBeatManager->setRuntimeExecutor(runtimeExecutor); - // TODO: T31905686 Create synchronous Event Beat jni::global_ref localJavaUIManager = javaUIManager_; EventBeat::Factory synchronousBeatFactory = diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventBeatManager.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventBeatManager.cpp index b92482e5f90..4117d5a74d7 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventBeatManager.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventBeatManager.cpp @@ -21,29 +21,23 @@ jni::local_ref EventBeatManager::initHybrid( return makeCxxInstance(jhybridobject); } -void EventBeatManager::setRuntimeExecutor(RuntimeExecutor runtimeExecutor) { - runtimeExecutor_ = runtimeExecutor; +void EventBeatManager::addObserver( + EventBeatManagerObserver const &observer) const { + std::lock_guard lock(mutex_); + observers_.insert(&observer); } -void EventBeatManager::registerEventBeat(EventBeat* eventBeat) const { +void EventBeatManager::removeObserver( + EventBeatManagerObserver const &observer) const { std::lock_guard lock(mutex_); - - registeredEventBeats_.insert(eventBeat); -} - -void EventBeatManager::unregisterEventBeat(EventBeat* eventBeat) const { - std::lock_guard lock(mutex_); - - registeredEventBeats_.erase(eventBeat); + observers_.erase(&observer); } void EventBeatManager::tick() { std::lock_guard lock(mutex_); - for (const auto eventBeat : registeredEventBeats_) { - runtimeExecutor_([=](jsi::Runtime &runtime) mutable { - eventBeat->beat(runtime); - }); + for (auto observer : observers_) { + observer->tick(); } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventBeatManager.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventBeatManager.h index 0c73dbda071..462e0704fb0 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventBeatManager.h +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventBeatManager.h @@ -17,33 +17,44 @@ namespace facebook { namespace react { +class EventBeatManagerObserver { + public: + /* + * Called by `EventBeatManager` on the main thread signaling that this is a + * good time to flush an event queue. + */ + virtual void tick() const = 0; + + virtual ~EventBeatManagerObserver() noexcept = default; +}; + class EventBeatManager : public jni::HybridClass { public: - constexpr static const char* const kJavaDescriptor = + constexpr static const char *const kJavaDescriptor = "Lcom/facebook/react/fabric/events/EventBeatManager;"; static void registerNatives(); - void setRuntimeExecutor(RuntimeExecutor runtimeExecutor); - - void registerEventBeat(EventBeat* eventBeat) const; - - void unregisterEventBeat(EventBeat* eventBeat) const; - EventBeatManager(jni::alias_ref jhybridobject); + /* + * Adds (or removes) observers. + * `EventBeatManager` does not own/retain observers; observers must overlive + * the manager or be properly removed before deallocation. + */ + void addObserver(EventBeatManagerObserver const &observer) const; + void removeObserver(EventBeatManagerObserver const &observer) const; + private: /* * Called by Java counterpart at the end of every run loop tick. */ void tick(); - RuntimeExecutor runtimeExecutor_; - jni::alias_ref jhybridobject_; - mutable std::unordered_set - registeredEventBeats_{}; // Protected by `mutex_` + mutable std::unordered_set + observers_{}; // Protected by `mutex_` mutable std::mutex mutex_;