From f30f8671730971c00da53525be4b4b019c2b3293 Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Thu, 18 Jan 2024 09:26:57 -0800 Subject: [PATCH] Refactor InspectorImpl internal page data structure (#42304) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/42304 Changelog: [Internal] Light refactor of `InspectorImpl`'s storage from two separate maps (one of them with tuples for values!) to a single map of objects. Reviewed By: hoxyq Differential Revision: D52786335 fbshipit-source-id: a49466ed7189fd032e486319bbdf77097a30885f --- .../React/Inspector/RCTInspector.mm | 2 +- .../src/main/jni/react/jni/JInspector.cpp | 3 +- .../chrome/tests/ConnectionDemuxTests.cpp | 4 +- .../InspectorInterfaces.cpp | 62 ++++++++++++++----- .../jsinspector-modern/InspectorInterfaces.h | 7 ++- 5 files changed, 57 insertions(+), 21 deletions(-) diff --git a/packages/react-native/React/Inspector/RCTInspector.mm b/packages/react-native/React/Inspector/RCTInspector.mm index 1c99fcbccf1..9385f5d19c3 100644 --- a/packages/react-native/React/Inspector/RCTInspector.mm +++ b/packages/react-native/React/Inspector/RCTInspector.mm @@ -67,7 +67,7 @@ RCT_NOT_IMPLEMENTED(-(instancetype)init) + (NSArray *)pages { - std::vector pages = getInstance()->getPages(); + std::vector pages = getInstance()->getPages(); NSMutableArray *array = [NSMutableArray arrayWithCapacity:pages.size()]; for (size_t i = 0; i < pages.size(); i++) { RCTInspectorPage *pageWrapper = [[RCTInspectorPage alloc] initWithId:pages[i].id diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/JInspector.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/jni/JInspector.cpp index 023db395e97..41e5c8f9707 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/jni/JInspector.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/jni/JInspector.cpp @@ -81,7 +81,8 @@ jni::global_ref JInspector::instance( } jni::local_ref> JInspector::getPages() { - std::vector pages = inspector_->getPages(); + std::vector pages = + inspector_->getPages(); auto array = jni::JArrayClass::newArray(pages.size()); for (size_t i = 0; i < pages.size(); i++) { (*array)[i] = JPage::create(pages[i].id, pages[i].title, pages[i].vm); diff --git a/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/tests/ConnectionDemuxTests.cpp b/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/tests/ConnectionDemuxTests.cpp index 7bf943fcd38..0e652e207b1 100644 --- a/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/tests/ConnectionDemuxTests.cpp +++ b/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/tests/ConnectionDemuxTests.cpp @@ -23,13 +23,13 @@ namespace inspector_modern { namespace chrome { using ::facebook::react::jsinspector_modern::IInspector; -using ::facebook::react::jsinspector_modern::InspectorPage; +using ::facebook::react::jsinspector_modern::InspectorPageDescription; using ::facebook::react::jsinspector_modern::IRemoteConnection; namespace { std::unordered_map makePageMap( - const std::vector& pages) { + const std::vector& pages) { std::unordered_map pageMap; for (auto& page : pages) { diff --git a/packages/react-native/ReactCommon/jsinspector-modern/InspectorInterfaces.cpp b/packages/react-native/ReactCommon/jsinspector-modern/InspectorInterfaces.cpp index ba054f9fa50..6b692be11da 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/InspectorInterfaces.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/InspectorInterfaces.cpp @@ -31,18 +31,53 @@ class InspectorImpl : public IInspector { ConnectFunc connectFunc) override; void removePage(int pageId) override; - std::vector getPages() const override; + std::vector getPages() const override; std::unique_ptr connect( int pageId, std::unique_ptr remote) override; private: + class Page { + public: + Page( + int id, + const std::string& title, + const std::string& vm, + ConnectFunc connectFunc); + operator InspectorPageDescription() const; + + ConnectFunc getConnectFunc() const; + + private: + int id_; + std::string title_; + std::string vm_; + ConnectFunc connectFunc_; + }; mutable std::mutex mutex_; int nextPageId_{1}; - std::unordered_map> titles_; - std::unordered_map connectFuncs_; + std::unordered_map pages_; }; +InspectorImpl::Page::Page( + int id, + const std::string& title, + const std::string& vm, + ConnectFunc connectFunc) + : id_(id), title_(title), vm_(vm), connectFunc_(std::move(connectFunc)) {} + +InspectorImpl::Page::operator InspectorPageDescription() const { + return InspectorPageDescription{ + .id = id_, + .title = title_, + .vm = vm_, + }; +} + +InspectorImpl::ConnectFunc InspectorImpl::Page::getConnectFunc() const { + return connectFunc_; +} + int InspectorImpl::addPage( const std::string& title, const std::string& vm, @@ -50,8 +85,7 @@ int InspectorImpl::addPage( std::scoped_lock lock(mutex_); int pageId = nextPageId_++; - titles_[pageId] = std::make_tuple(title, vm); - connectFuncs_[pageId] = std::move(connectFunc); + pages_.emplace(pageId, Page{pageId, title, vm, std::move(connectFunc)}); return pageId; } @@ -59,17 +93,15 @@ int InspectorImpl::addPage( void InspectorImpl::removePage(int pageId) { std::scoped_lock lock(mutex_); - titles_.erase(pageId); - connectFuncs_.erase(pageId); + pages_.erase(pageId); } -std::vector InspectorImpl::getPages() const { +std::vector InspectorImpl::getPages() const { std::scoped_lock lock(mutex_); - std::vector inspectorPages; - for (auto& it : titles_) { - inspectorPages.push_back(InspectorPage{ - it.first, std::get<0>(it.second), std::get<1>(it.second)}); + std::vector inspectorPages; + for (auto& it : pages_) { + inspectorPages.push_back(InspectorPageDescription(it.second)); } return inspectorPages; @@ -83,9 +115,9 @@ std::unique_ptr InspectorImpl::connect( { std::scoped_lock lock(mutex_); - auto it = connectFuncs_.find(pageId); - if (it != connectFuncs_.end()) { - connectFunc = it->second; + auto it = pages_.find(pageId); + if (it != pages_.end()) { + connectFunc = it->second.getConnectFunc(); } } diff --git a/packages/react-native/ReactCommon/jsinspector-modern/InspectorInterfaces.h b/packages/react-native/ReactCommon/jsinspector-modern/InspectorInterfaces.h index 3bd91918bb9..0a27198d899 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/InspectorInterfaces.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/InspectorInterfaces.h @@ -31,12 +31,15 @@ class IDestructible { virtual ~IDestructible() = 0; }; -struct InspectorPage { +struct InspectorPageDescription { const int id; const std::string title; const std::string vm; }; +// Alias for backwards compatibility. +using InspectorPage = InspectorPageDescription; + /// IRemoteConnection allows the VM to send debugger messages to the client. class JSINSPECTOR_EXPORT IRemoteConnection : public IDestructible { public: @@ -72,7 +75,7 @@ class JSINSPECTOR_EXPORT IInspector : public IDestructible { virtual void removePage(int pageId) = 0; /// getPages is called by the client to list all debuggable pages. - virtual std::vector getPages() const = 0; + virtual std::vector getPages() const = 0; /// connect is called by the client to initiate a debugging session on the /// given page.