From 266122248f16566f4e436cae7f95a75e9d03c19d Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Wed, 14 Feb 2024 07:13:19 -0800 Subject: [PATCH] =?UTF-8?q?RuntimeTarget=20refactor=20-=20introduce=20Weak?= =?UTF-8?q?List=20for=20Target=E2=86=92Agent=20refs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Changelog: [Internal] Replaces the copypasta'd `PageTarget::forEachSession`, `InstanceTarget::forEachAgent` and `RuntimeTarget::forEachAgent` with a shared utility class for managing a list of `weak_ptr`s. In a `WeakList`, elements can only be added (`insert`), iterated over (`forEach`), and counted (`size`, `empty`). Conceptually, elements are automatically removed from the list as soon as they're destroyed, but internally, space is reclaimed *lazily*: the next time we have a reason to iterate over the underlying list, we delete any pointers that are found to be null. ## Naming This is almost a WeakSet, but we don't bother checking for duplicates, hence "WeakList". Reviewed By: hoxyq Differential Revision: D53671483 fbshipit-source-id: 460bfbaa2b8e821281dc352fed0946f99352c9fc --- .../jsinspector-modern/InstanceTarget.cpp | 14 +-- .../jsinspector-modern/InstanceTarget.h | 21 +---- .../jsinspector-modern/PageTarget.cpp | 13 +-- .../jsinspector-modern/PageTarget.h | 22 +---- .../jsinspector-modern/RuntimeTarget.cpp | 9 +- .../jsinspector-modern/RuntimeTarget.h | 22 +---- .../ReactCommon/jsinspector-modern/WeakList.h | 79 ++++++++++++++++ .../jsinspector-modern/tests/WeakListTest.cpp | 91 +++++++++++++++++++ 8 files changed, 184 insertions(+), 87 deletions(-) create mode 100644 packages/react-native/ReactCommon/jsinspector-modern/WeakList.h create mode 100644 packages/react-native/ReactCommon/jsinspector-modern/tests/WeakListTest.cpp diff --git a/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.cpp b/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.cpp index 060045c2703..8030ec740d3 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.cpp @@ -33,18 +33,11 @@ std::shared_ptr InstanceTarget::createAgent( auto instanceAgent = std::make_shared(channel, *this, sessionState); instanceAgent->setCurrentRuntime(currentRuntime_.get()); - agents_.push_back(instanceAgent); + agents_.insert(instanceAgent); return instanceAgent; } -void InstanceTarget::removeExpiredAgents() { - // Remove all expired agents. - forEachAgent([](auto&) {}); -} - InstanceTarget::~InstanceTarget() { - removeExpiredAgents(); - // Agents are owned by the session, not by InstanceTarget, but // they hold an InstanceTarget& that we must guarantee is valid. assert( @@ -59,7 +52,7 @@ RuntimeTarget& InstanceTarget::registerRuntime( currentRuntime_ = RuntimeTarget::create( delegate, jsExecutor, makeVoidExecutor(executorFromThis())); - forEachAgent([currentRuntime = &*currentRuntime_](InstanceAgent& agent) { + agents_.forEach([currentRuntime = &*currentRuntime_](InstanceAgent& agent) { agent.setCurrentRuntime(currentRuntime); }); return *currentRuntime_; @@ -69,7 +62,8 @@ void InstanceTarget::unregisterRuntime(RuntimeTarget& Runtime) { assert( currentRuntime_ && currentRuntime_.get() == &Runtime && "Invalid unregistration"); - forEachAgent([](InstanceAgent& agent) { agent.setCurrentRuntime(nullptr); }); + agents_.forEach( + [](InstanceAgent& agent) { agent.setCurrentRuntime(nullptr); }); currentRuntime_.reset(); } diff --git a/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.h b/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.h index 266dc5dec87..a96197e4574 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.h @@ -10,6 +10,7 @@ #include "RuntimeTarget.h" #include "ScopedExecutor.h" #include "SessionState.h" +#include "WeakList.h" #include #include @@ -83,25 +84,7 @@ class InstanceTarget : public EnableExecutorFromThis { InstanceTargetDelegate& delegate_; std::shared_ptr currentRuntime_{nullptr}; - std::list> agents_; - - /** - * Call the given function for every active agent, and clean up any - * references to inactive agents. - */ - template - void forEachAgent(Fn&& fn) { - for (auto it = agents_.begin(); it != agents_.end();) { - if (auto agent = it->lock()) { - fn(*agent); - ++it; - } else { - it = agents_.erase(it); - } - } - } - - void removeExpiredAgents(); + WeakList agents_; }; } // namespace facebook::react::jsinspector_modern diff --git a/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.cpp b/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.cpp index 2b3d32fd0ab..78167b16658 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.cpp @@ -115,19 +115,12 @@ std::unique_ptr PageTarget::connect( auto session = std::make_shared( std::move(connectionToFrontend), controller_, std::move(sessionMetadata)); session->setCurrentInstance(currentInstance_.get()); - sessions_.push_back(std::weak_ptr(session)); + sessions_.insert(std::weak_ptr(session)); return std::make_unique( [session](std::string message) { (*session)(message); }); } -void PageTarget::removeExpiredSessions() { - // Remove all expired sessions. - forEachSession([](auto&) {}); -} - PageTarget::~PageTarget() { - removeExpiredSessions(); - // Sessions are owned by InspectorPackagerConnection, not by PageTarget, but // they hold a PageTarget& that we must guarantee is valid. assert( @@ -141,7 +134,7 @@ InstanceTarget& PageTarget::registerInstance(InstanceTargetDelegate& delegate) { assert(!currentInstance_ && "Only one instance allowed"); currentInstance_ = InstanceTarget::create(delegate, makeVoidExecutor(executorFromThis())); - forEachSession( + sessions_.forEach( [currentInstance = &*currentInstance_](PageTargetSession& session) { session.setCurrentInstance(currentInstance); }); @@ -152,7 +145,7 @@ void PageTarget::unregisterInstance(InstanceTarget& instance) { assert( currentInstance_ && currentInstance_.get() == &instance && "Invalid unregistration"); - forEachSession( + sessions_.forEach( [](PageTargetSession& session) { session.setCurrentInstance(nullptr); }); currentInstance_.reset(); } diff --git a/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.h b/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.h index c1d874a68c3..90c6513ba70 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.h @@ -8,11 +8,11 @@ #pragma once #include "ScopedExecutor.h" +#include "WeakList.h" #include #include -#include #include #include @@ -164,28 +164,10 @@ class JSINSPECTOR_EXPORT PageTarget PageTarget(PageTargetDelegate& delegate); PageTargetDelegate& delegate_; - std::list> sessions_; + WeakList sessions_; PageTargetController controller_{*this}; std::shared_ptr currentInstance_{nullptr}; - /** - * Call the given function for every active session, and clean up any - * references to inactive sessions. - */ - template - void forEachSession(Fn&& fn) { - for (auto it = sessions_.begin(); it != sessions_.end();) { - if (auto session = it->lock()) { - fn(*session); - ++it; - } else { - it = sessions_.erase(it); - } - } - } - - void removeExpiredSessions(); - inline PageTargetDelegate& getDelegate() { return delegate_; } diff --git a/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.cpp b/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.cpp index 67735136574..52cdc0516c2 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.cpp @@ -32,18 +32,11 @@ std::shared_ptr RuntimeTarget::createAgent( *this, sessionState, delegate_.createAgentDelegate(channel, sessionState)); - agents_.push_back(runtimeAgent); + agents_.insert(runtimeAgent); return runtimeAgent; } -void RuntimeTarget::removeExpiredAgents() { - // Remove all expired agents. - forEachAgent([](auto&) {}); -} - RuntimeTarget::~RuntimeTarget() { - removeExpiredAgents(); - // Agents are owned by the session, not by RuntimeTarget, but // they hold a RuntimeTarget& that we must guarantee is valid. assert( diff --git a/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.h b/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.h index 6a954a05f1d..984a38a29b7 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.h @@ -12,8 +12,8 @@ #include "RuntimeAgent.h" #include "ScopedExecutor.h" #include "SessionState.h" +#include "WeakList.h" -#include #include #ifndef JSINSPECTOR_EXPORT @@ -106,25 +106,7 @@ class JSINSPECTOR_EXPORT RuntimeTarget RuntimeTargetDelegate& delegate_; RuntimeExecutor jsExecutor_; - std::list> agents_; - - /** - * Call the given function for every active agent, and clean up any - * references to inactive agents. - */ - template - void forEachAgent(Fn&& fn) { - for (auto it = agents_.begin(); it != agents_.end();) { - if (auto agent = it->lock()) { - fn(*agent); - ++it; - } else { - it = agents_.erase(it); - } - } - } - - void removeExpiredAgents(); + WeakList agents_; }; } // namespace facebook::react::jsinspector_modern diff --git a/packages/react-native/ReactCommon/jsinspector-modern/WeakList.h b/packages/react-native/ReactCommon/jsinspector-modern/WeakList.h new file mode 100644 index 00000000000..4e097c67b13 --- /dev/null +++ b/packages/react-native/ReactCommon/jsinspector-modern/WeakList.h @@ -0,0 +1,79 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +#pragma once + +#include +#include + +namespace facebook::react::jsinspector_modern { + +/** + * A list that holds weak pointers to objects of type `T`. Null pointers are not + * considered to be in the list. + * + * The list is not thread-safe! The caller is responsible for synchronization. + */ +template +class WeakList { + public: + /** + * Call the given function for every element in the list, ensuring the element + * is not destroyed for the duration of the call. Elements are visited in the + * order they were inserted. + * + * As a side effect, any null pointers in the underlying list (corresponding + * to destroyed elements) will be removed during iteration. + */ + template + void forEach(Fn&& fn) const { + for (auto it = ptrs_.begin(); it != ptrs_.end();) { + if (auto ptr = it->lock()) { + fn(*ptr); + ++it; + } else { + it = ptrs_.erase(it); + } + } + } + + /** + * Returns the number of (non-null) elements in the list. The count will only + * remain accurate as long as the list is not modified and elements are + * not destroyed. + * + * As a side effect, any null pointers in the underlying list (corresponding + * to destroyed elements) will be removed during this method. + */ + size_t size() const { + size_t count{0}; + forEach([&count](const auto&) { ++count; }); + return count; + } + + /** + * Returns true if there are no elements in the list. + * + * As a side effect, any null pointers in the underlying list (corresponding + * to destroyed elements) will be removed during this method. + */ + bool empty() const { + return !size(); + } + + /** + * Inserts an element into the list. + */ + void insert(std::weak_ptr ptr) { + ptrs_.push_back(ptr); + } + + private: + mutable std::list> ptrs_; +}; + +} // namespace facebook::react::jsinspector_modern diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/WeakListTest.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tests/WeakListTest.cpp new file mode 100644 index 00000000000..ee86a8e68a9 --- /dev/null +++ b/packages/react-native/ReactCommon/jsinspector-modern/tests/WeakListTest.cpp @@ -0,0 +1,91 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +#include + +#include +#include + +#include + +using namespace ::testing; + +namespace facebook::react::jsinspector_modern { + +TEST(WeakListTest, Size) { + WeakList list; + EXPECT_EQ(list.size(), 0); + + auto p1 = std::make_shared(1); + list.insert(p1); + EXPECT_EQ(list.size(), 1); + + auto p2 = std::make_shared(2); + list.insert(p2); + EXPECT_EQ(list.size(), 2); + + p1.reset(); + EXPECT_EQ(list.size(), 1); + + p2.reset(); + EXPECT_EQ(list.size(), 0); +} + +TEST(WeakListTest, Empty) { + WeakList list; + EXPECT_EQ(list.empty(), true); + + auto p1 = std::make_shared(1); + list.insert(p1); + EXPECT_EQ(list.empty(), false); + + auto p2 = std::make_shared(2); + list.insert(p2); + EXPECT_EQ(list.empty(), false); + + p1.reset(); + EXPECT_EQ(list.empty(), false); + + p2.reset(); + EXPECT_EQ(list.empty(), true); +} + +TEST(WeakListTest, ForEach) { + WeakList list; + auto p1 = std::make_shared(1); + list.insert(p1); + auto p2 = std::make_shared(2); + list.insert(p2); + auto p3 = std::make_shared(3); + list.insert(p3); + + p2.reset(); + + std::vector visited; + list.forEach([&visited](const int& value) { visited.push_back(value); }); + EXPECT_THAT(visited, ElementsAre(1, 3)); +} + +TEST(WeakListTest, ElementsAreAliveDuringCallback) { + WeakList list; + auto p1 = std::make_shared(1); + // A separate weak_ptr to observe the lifetime of `p1`. + std::weak_ptr wp1 = p1; + list.insert(p1); + + std::vector visited; + list.forEach([&](const int& value) { + p1.reset(); + EXPECT_FALSE(wp1.expired()); + visited.push_back(value); + }); + + EXPECT_TRUE(wp1.expired()); + EXPECT_THAT(visited, ElementsAre(1)); +} + +} // namespace facebook::react::jsinspector_modern