Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: reduce platform worker barrier lifetime #23419

Closed

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Oct 11, 2018

Minor cleanup in the lifetime for the platform worker initialization
synchronization barrier.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

CI: https://ci.nodejs.org/job/node-test-pull-request/17740/

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 11, 2018
src/node_platform.cc Outdated Show resolved Hide resolved
Minor cleanup in the lifetime for the platform worker initialization
synchronization barrier.
@ofrobots ofrobots added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 11, 2018
@ofrobots
Copy link
Contributor Author

Landed in 9f7e3a4.

@ofrobots ofrobots closed this Oct 13, 2018
@ofrobots ofrobots deleted the platform-worker-fields-scope branch October 13, 2018 05:31
ofrobots added a commit that referenced this pull request Oct 13, 2018
Minor cleanup in the lifetime for the platform worker initialization
synchronization barrier.

PR-URL: #23419
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@BridgeAR
Copy link
Member

Should this be backported to 10.x? If so, a separate backport PR is needed.

Diff
diff --cc src/node_platform.cc
index ad0cd0db3e,047cb43268..0000000000
--- a/src/node_platform.cc
+++ b/src/node_platform.cc
@@@ -161,22 -166,27 +161,36 @@@ class BackgroundTaskRunner::DelayedTask
    std::unordered_set<uv_timer_t*> timers_;
  };
  
++<<<<<<< HEAD
 +BackgroundTaskRunner::BackgroundTaskRunner(int thread_pool_size) {
 +  Mutex::ScopedLock lock(platform_workers_mutex_);
 +  pending_platform_workers_ = thread_pool_size;
++=======
+ WorkerThreadsTaskRunner::WorkerThreadsTaskRunner(int thread_pool_size) {
+   Mutex platform_workers_mutex;
+   ConditionVariable platform_workers_ready;
+ 
+   Mutex::ScopedLock lock(platform_workers_mutex);
+   int pending_platform_workers = thread_pool_size;
++>>>>>>> 9f7e3a4040... src: reduce platform worker barrier lifetime
  
    delayed_task_scheduler_.reset(
 -      new DelayedTaskScheduler(&pending_worker_tasks_));
 +      new DelayedTaskScheduler(&background_tasks_));
    threads_.push_back(delayed_task_scheduler_->Start());
  
    for (int i = 0; i < thread_pool_size; i++) {
      PlatformWorkerData* worker_data = new PlatformWorkerData{
++<<<<<<< HEAD
 +      &background_tasks_, &platform_workers_mutex_,
 +      &platform_workers_ready_, &pending_platform_workers_, i
++=======
+       &pending_worker_tasks_, &platform_workers_mutex,
+       &platform_workers_ready, &pending_platform_workers, i
++>>>>>>> 9f7e3a4040... src: reduce platform worker barrier lifetime
      };
      std::unique_ptr<uv_thread_t> t { new uv_thread_t() };
 -    if (uv_thread_create(t.get(), PlatformWorkerThread,
 -                         worker_data) != 0) {
 +    if (uv_thread_create(t.get(), BackgroundRunner, worker_data) != 0)
        break;
 -    }
      threads_.push_back(std::move(t));
    }

@BridgeAR BridgeAR modified the milestone: 10 Oct 17, 2018
jasnell pushed a commit that referenced this pull request Oct 17, 2018
Minor cleanup in the lifetime for the platform worker initialization
synchronization barrier.

PR-URL: #23419
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@ofrobots
Copy link
Contributor Author

There is no pressing need to backport it, but I can if necessary (to unblock other backports).

andrewhughes101 pushed a commit to andrewhughes101/node that referenced this pull request Jul 29, 2019
Minor cleanup in the lifetime for the platform worker initialization
synchronization barrier.

PR-URL: nodejs#23419
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
BethGriggs pushed a commit that referenced this pull request Sep 20, 2019
Minor cleanup in the lifetime for the platform worker initialization
synchronization barrier.

PR-URL: #23419
Backport-PR-URL: #28844
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Oct 7, 2019
@addaleax addaleax added the v8 platform Issues and PRs related to Node's v8::Platform implementation. label Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. v8 platform Issues and PRs related to Node's v8::Platform implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.