From c119b1ebef0afe3b8dbedc4f869a1c67684d6009 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Wed, 30 Mar 2022 12:09:42 +0900 Subject: [PATCH] chore: remove redundant code in node integration (#33500) --- shell/browser/electron_browser_main_parts.cc | 4 ++-- shell/common/node_bindings.cc | 21 ++++++++++++++-- shell/common/node_bindings.h | 11 +++++---- shell/common/node_bindings_linux.cc | 24 ------------------- shell/common/node_bindings_linux.h | 7 ------ shell/common/node_bindings_mac.cc | 24 ------------------- shell/common/node_bindings_mac.h | 7 ------ shell/common/node_bindings_win.cc | 25 -------------------- shell/common/node_bindings_win.h | 7 ------ shell/renderer/electron_renderer_client.cc | 6 ++--- shell/renderer/web_worker_observer.cc | 4 ++-- 11 files changed, 32 insertions(+), 108 deletions(-) diff --git a/shell/browser/electron_browser_main_parts.cc b/shell/browser/electron_browser_main_parts.cc index 82713b7d1528..98344e65d95a 100644 --- a/shell/browser/electron_browser_main_parts.cc +++ b/shell/browser/electron_browser_main_parts.cc @@ -402,8 +402,8 @@ void ElectronBrowserMainParts::ToolkitInitialized() { int ElectronBrowserMainParts::PreMainMessageLoopRun() { // Run user's main script before most things get initialized, so we can have // a chance to setup everything. - node_bindings_->PrepareMessageLoop(); - node_bindings_->RunMessageLoop(); + node_bindings_->PrepareEmbedThread(); + node_bindings_->StartPolling(); // url::Add*Scheme are not threadsafe, this helps prevent data races. url::LockSchemeRegistries(); diff --git a/shell/common/node_bindings.cc b/shell/common/node_bindings.cc index c83253dc9210..15560da0cb71 100644 --- a/shell/common/node_bindings.cc +++ b/shell/common/node_bindings.cc @@ -584,7 +584,16 @@ void NodeBindings::LoadEnvironment(node::Environment* env) { gin_helper::EmitEvent(env->isolate(), env->process_object(), "loaded"); } -void NodeBindings::PrepareMessageLoop() { +void NodeBindings::PrepareEmbedThread() { + // IOCP does not change for the process until the loop is recreated, + // we ensure that there is only a single polling thread satisfying + // the concurrency limit set from CreateIoCompletionPort call by + // uv_loop_init for the lifetime of this process. + // More background can be found at: + // https://github.com/microsoft/vscode/issues/142786#issuecomment-1061673400 + if (initialized_) + return; + // Add dummy handle for libuv, otherwise libuv would quit when there is // nothing to do. uv_async_init(uv_loop_, dummy_uv_handle_.get(), nullptr); @@ -594,7 +603,15 @@ void NodeBindings::PrepareMessageLoop() { uv_thread_create(&embed_thread_, EmbedThreadRunner, this); } -void NodeBindings::RunMessageLoop() { +void NodeBindings::StartPolling() { + // Avoid calling UvRunOnce if the loop is already active, + // otherwise it can lead to situations were the number of active + // threads processing on IOCP is greater than the concurrency limit. + if (initialized_) + return; + + initialized_ = true; + // The MessageLoop should have been created, remember the one in main thread. task_runner_ = base::ThreadTaskRunnerHandle::Get(); diff --git a/shell/common/node_bindings.h b/shell/common/node_bindings.h index 204cf12d5cc6..54bed07a39aa 100644 --- a/shell/common/node_bindings.h +++ b/shell/common/node_bindings.h @@ -92,11 +92,11 @@ class NodeBindings { // Load node.js in the environment. void LoadEnvironment(node::Environment* env); - // Prepare for message loop integration. - virtual void PrepareMessageLoop(); + // Prepare embed thread for message loop integration. + void PrepareEmbedThread(); - // Do message loop integration. - virtual void RunMessageLoop(); + // Notify embed thread to start polling after environment is loaded. + void StartPolling(); // Gets/sets the per isolate data. void set_isolate_data(node::IsolateData* isolate_data) { @@ -144,6 +144,9 @@ class NodeBindings { // Thread to poll uv events. static void EmbedThreadRunner(void* arg); + // Indicates whether polling thread has been created. + bool initialized_ = false; + // Whether the libuv loop has ended. bool embed_closed_ = false; diff --git a/shell/common/node_bindings_linux.cc b/shell/common/node_bindings_linux.cc index 395dcfc28b48..5fc157da00e5 100644 --- a/shell/common/node_bindings_linux.cc +++ b/shell/common/node_bindings_linux.cc @@ -17,30 +17,6 @@ NodeBindingsLinux::NodeBindingsLinux(BrowserEnvironment browser_env) epoll_ctl(epoll_, EPOLL_CTL_ADD, backend_fd, &ev); } -NodeBindingsLinux::~NodeBindingsLinux() = default; - -void NodeBindingsLinux::PrepareMessageLoop() { - int handle = uv_backend_fd(uv_loop_); - - // If the backend fd hasn't changed, don't proceed. - if (handle == handle_) - return; - - NodeBindings::PrepareMessageLoop(); -} - -void NodeBindingsLinux::RunMessageLoop() { - int handle = uv_backend_fd(uv_loop_); - - // If the backend fd hasn't changed, don't proceed. - if (handle == handle_) - return; - - handle_ = handle; - - NodeBindings::RunMessageLoop(); -} - void NodeBindingsLinux::PollEvents() { int timeout = uv_backend_timeout(uv_loop_); diff --git a/shell/common/node_bindings_linux.h b/shell/common/node_bindings_linux.h index 4499f1c754fe..db8a8ed32ad5 100644 --- a/shell/common/node_bindings_linux.h +++ b/shell/common/node_bindings_linux.h @@ -13,19 +13,12 @@ namespace electron { class NodeBindingsLinux : public NodeBindings { public: explicit NodeBindingsLinux(BrowserEnvironment browser_env); - ~NodeBindingsLinux() override; - - void PrepareMessageLoop() override; - void RunMessageLoop() override; private: void PollEvents() override; // Epoll to poll for uv's backend fd. int epoll_; - - // uv's backend fd. - int handle_ = -1; }; } // namespace electron diff --git a/shell/common/node_bindings_mac.cc b/shell/common/node_bindings_mac.cc index cfc37e5f5013..fe9b0d698619 100644 --- a/shell/common/node_bindings_mac.cc +++ b/shell/common/node_bindings_mac.cc @@ -17,30 +17,6 @@ namespace electron { NodeBindingsMac::NodeBindingsMac(BrowserEnvironment browser_env) : NodeBindings(browser_env) {} -NodeBindingsMac::~NodeBindingsMac() = default; - -void NodeBindingsMac::PrepareMessageLoop() { - int handle = uv_backend_fd(uv_loop_); - - // If the backend fd hasn't changed, don't proceed. - if (handle == handle_) - return; - - NodeBindings::PrepareMessageLoop(); -} - -void NodeBindingsMac::RunMessageLoop() { - int handle = uv_backend_fd(uv_loop_); - - // If the backend fd hasn't changed, don't proceed. - if (handle == handle_) - return; - - handle_ = handle; - - NodeBindings::RunMessageLoop(); -} - void NodeBindingsMac::PollEvents() { struct timeval tv; int timeout = uv_backend_timeout(uv_loop_); diff --git a/shell/common/node_bindings_mac.h b/shell/common/node_bindings_mac.h index 911287a65fd3..cae9540edbe7 100644 --- a/shell/common/node_bindings_mac.h +++ b/shell/common/node_bindings_mac.h @@ -13,16 +13,9 @@ namespace electron { class NodeBindingsMac : public NodeBindings { public: explicit NodeBindingsMac(BrowserEnvironment browser_env); - ~NodeBindingsMac() override; - - void PrepareMessageLoop() override; - void RunMessageLoop() override; private: void PollEvents() override; - - // uv's backend fd. - int handle_ = -1; }; } // namespace electron diff --git a/shell/common/node_bindings_win.cc b/shell/common/node_bindings_win.cc index 1410925f195c..9b2319cfdc86 100644 --- a/shell/common/node_bindings_win.cc +++ b/shell/common/node_bindings_win.cc @@ -27,31 +27,6 @@ NodeBindingsWin::NodeBindingsWin(BrowserEnvironment browser_env) } } -NodeBindingsWin::~NodeBindingsWin() = default; - -void NodeBindingsWin::PrepareMessageLoop() { - // IOCP does not change for the process until the loop is recreated, - // we ensure that there is only a single polling thread satisfying - // the concurrency limit set from CreateIoCompletionPort call by - // uv_loop_init for the lifetime of this process. - if (initialized_) - return; - - NodeBindings::PrepareMessageLoop(); -} - -void NodeBindingsWin::RunMessageLoop() { - // Avoid calling UvRunOnce if the loop is already active, - // otherwise it can lead to situations were the number of active - // threads processing on IOCP is greater than the concurrency limit. - if (initialized_) - return; - - initialized_ = true; - - NodeBindings::RunMessageLoop(); -} - void NodeBindingsWin::PollEvents() { // If there are other kinds of events pending, uv_backend_timeout will // instruct us not to wait. diff --git a/shell/common/node_bindings_win.h b/shell/common/node_bindings_win.h index 59d7469b0ff7..a0dcd3378f90 100644 --- a/shell/common/node_bindings_win.h +++ b/shell/common/node_bindings_win.h @@ -13,16 +13,9 @@ namespace electron { class NodeBindingsWin : public NodeBindings { public: explicit NodeBindingsWin(BrowserEnvironment browser_env); - ~NodeBindingsWin() override; - - void PrepareMessageLoop() override; - void RunMessageLoop() override; private: void PollEvents() override; - - // Indicates whether polling thread has been created. - bool initialized_ = false; }; } // namespace electron diff --git a/shell/renderer/electron_renderer_client.cc b/shell/renderer/electron_renderer_client.cc index 08276425938f..1c819d13c445 100644 --- a/shell/renderer/electron_renderer_client.cc +++ b/shell/renderer/electron_renderer_client.cc @@ -93,9 +93,7 @@ void ElectronRendererClient::DidCreateScriptContext( if (!node_integration_initialized_) { node_integration_initialized_ = true; node_bindings_->Initialize(); - node_bindings_->PrepareMessageLoop(); - } else { - node_bindings_->PrepareMessageLoop(); + node_bindings_->PrepareEmbedThread(); } // Setup node tracing controller. @@ -131,7 +129,7 @@ void ElectronRendererClient::DidCreateScriptContext( node_bindings_->set_uv_env(env); // Give the node loop a run to make sure everything is ready. - node_bindings_->RunMessageLoop(); + node_bindings_->StartPolling(); } } diff --git a/shell/renderer/web_worker_observer.cc b/shell/renderer/web_worker_observer.cc index ca929bf9e4bb..ea5f60daba42 100644 --- a/shell/renderer/web_worker_observer.cc +++ b/shell/renderer/web_worker_observer.cc @@ -56,7 +56,7 @@ void WebWorkerObserver::WorkerScriptReadyForEvaluation( isolate, v8::MicrotasksScope::kDoNotRunMicrotasks); // Start the embed thread. - node_bindings_->PrepareMessageLoop(); + node_bindings_->PrepareEmbedThread(); // Setup node tracing controller. if (!node::tracing::TraceEventHelper::GetAgent()) @@ -78,7 +78,7 @@ void WebWorkerObserver::WorkerScriptReadyForEvaluation( node_bindings_->set_uv_env(env); // Give the node loop a run to make sure everything is ready. - node_bindings_->RunMessageLoop(); + node_bindings_->StartPolling(); } void WebWorkerObserver::ContextWillDestroy(v8::Local context) {