[xray] Start actor workers in parallel #2168

pcmoritz · 2018-05-30T23:07:05Z

What do these changes do?

Related issue number

robertnishihara · 2018-05-30T23:12:45Z

src/ray/raylet/node_manager.cc

@@ -622,6 +622,10 @@ void NodeManager::AssignTask(Task &task) {
      // There are no more non-actor workers available to execute this task.
      // Start a new worker.
      worker_pool_.StartWorker();
+    } else if (spec.IsActorCreationTask()) {


Don't actor creation tasks already go through the if statement, so they won't hit the else if?

robertnishihara · 2018-05-30T23:13:30Z

src/ray/raylet/node_manager.cc

+    } else if (spec.IsActorCreationTask()) {
+      // We will need a new worker for this actor, so force start one.
+      // This parallelizes starting lots of actor workers.
+      worker_pool_.StartWorker(true);


Why does this parallelize starting actor workers? It seems to be serial.

AmplabJenkins · 2018-05-31T00:15:33Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5739/
Test PASSed.

AmplabJenkins · 2018-05-31T05:50:08Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5761/
Test FAILed.

AmplabJenkins · 2018-05-31T07:22:31Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5766/
Test PASSed.

pcmoritz · 2018-05-31T08:32:30Z

With the following program:

import ray
import time

ray.init(use_raylet=True)

@ray.remote
class Actor(object):
    def __init__(self):
        pass
    def f(self):
        pass

%time As = [Actor.remote() for i in range(64)]; ray.get([A.f.remote() for A in As])

On a m4.4xlarge (8 physical cores) we get these timings:

Without the PR: 27.1 s
With the PR: 3.4 s

cc @stephanie-wang

AmplabJenkins · 2018-05-31T09:10:08Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5772/
Test PASSed.

AmplabJenkins · 2018-05-31T09:32:26Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5776/
Test PASSed.

robertnishihara · 2018-05-31T18:57:31Z

src/ray/raylet/worker_pool.cc

-  if (!started_worker_pids_.empty() && !force_start) {
+  // The first condition makes sure that we are always starting up to
+  // std::thread::hardware_concurrency() number of processes in parallel.
+  if (NumStartedWorkers() > std::thread::hardware_concurrency() && !force_start) {


Instead of std::thread::hardware_concurrency(), let's use num_cpus

AmplabJenkins · 2018-05-31T19:40:34Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5780/
Test FAILed.

AmplabJenkins · 2018-05-31T20:29:29Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5782/
Test FAILed.

AmplabJenkins · 2018-05-31T21:04:20Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5781/
Test PASSed.

pcmoritz · 2018-05-31T21:06:28Z

retest this please

AmplabJenkins · 2018-05-31T22:17:03Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5783/
Test PASSed.

stephanie-wang

Looks good! Left a few small comments.

stephanie-wang · 2018-06-01T00:05:55Z

src/ray/raylet/worker_pool.cc

@@ -155,7 +158,7 @@ bool WorkerPool::DisconnectWorker(std::shared_ptr<Worker> worker) {
 // Protected WorkerPool methods.
 void WorkerPool::AddStartedWorker(pid_t pid) { started_worker_pids_.insert(pid); }

-uint32_t WorkerPool::NumStartedWorkers() const { return started_worker_pids_.size(); }
+int WorkerPool::NumStartedWorkers() const { return started_worker_pids_.size(); }


Why change this to an int?

See https://google.github.io/styleguide/cppguide.html, "On Unsigned Integers"

In this case it was necessary to avoid a signed/unsigned mismatch.

In general it's better to use signed ints, unless there is a good reason not to (i.e. for doing bit manipulations or modular arithmetic).

stephanie-wang · 2018-06-01T00:06:42Z

src/ray/raylet/worker_pool.cc

-  if (!started_worker_pids_.empty() && !force_start) {
+  // The first condition makes sure that we are always starting up to
+  // num_cpus_ number of processes in parallel.
+  if (NumStartedWorkers() > num_cpus_ && !force_start) {


Can you update the documentation for StartWorker to account for the new logic?

Yeah, the documentation should also make clear that StartWorker may not actually start a worker.

stephanie-wang · 2018-06-01T00:09:13Z

src/ray/raylet/scheduling_resources.cc

@@ -116,6 +116,12 @@ bool ResourceSet::GetResource(const std::string &resource_name, double *value) c
  return true;
 }

+double ResourceSet::GetNumCPUs() const {


There are a couple other places in the code where we have to do this logic (I think in node_manager.cc, maybe elsewhere too). Can you find and replace that code with the GetNumCPUs method?

stephanie-wang · 2018-06-01T00:09:35Z

src/ray/raylet/scheduling_resources.h

+  /// Return the number of CPUs.
+  ///
+  /// \return Number of CPUs.
+  double GetNumCPUs() const;


I think the convention we've been using is to lower-case acronyms, so it should be GetNumCpus.

robertnishihara · 2018-06-01T00:19:24Z

src/ray/raylet/node_manager.cc

@@ -70,7 +70,9 @@ NodeManager::NodeManager(boost::asio::io_service &io_service,
      heartbeat_timer_(io_service),
      heartbeat_period_ms_(config.heartbeat_period_ms),
      local_resources_(config.resource_config),
-      worker_pool_(config.num_initial_workers, config.worker_command),
+      worker_pool_(config.num_initial_workers,
+                   static_cast<int>(config.resource_config.GetNumCPUs()) + 1,


AmplabJenkins · 2018-06-01T04:16:09Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5789/
Test FAILed.

AmplabJenkins · 2018-06-01T21:53:06Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5812/
Test FAILed.

pcmoritz · 2018-06-01T21:56:00Z

Jenkins retest this please

AmplabJenkins · 2018-06-01T22:28:46Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5817/
Test FAILed.

pcmoritz · 2018-06-01T22:50:21Z

Jenkins retest this please

AmplabJenkins · 2018-06-02T00:04:49Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5819/
Test PASSed.

start actor workers in parallel

9e07dc2

pcmoritz changed the title ~~[WIP] Start actor workers in parallel~~ [WIP] [xray] Start actor workers in parallel May 30, 2018

robertnishihara reviewed May 30, 2018

View reviewed changes

fix

75aa7c0

pcmoritz force-pushed the start-actor-workers-parallel branch from 7c87761 to 75aa7c0 Compare May 31, 2018 06:09

pcmoritz added 2 commits May 31, 2018 00:59

simplify

acab21c

fix

f83990c

pcmoritz changed the title ~~[WIP] [xray] Start actor workers in parallel~~ [xray] Start actor workers in parallel May 31, 2018

robertnishihara reviewed May 31, 2018

View reviewed changes

use num_cpus

856b31f

pcmoritz added 2 commits May 31, 2018 12:53

fix

ca22057

linting

7a3e234

stephanie-wang reviewed Jun 1, 2018

View reviewed changes

robertnishihara reviewed Jun 1, 2018

View reviewed changes

temp commit

43b3f73

update

d39bcfb

robertnishihara approved these changes Jun 1, 2018

View reviewed changes

robertnishihara approved these changes Jun 2, 2018

View reviewed changes

robertnishihara merged commit e1024d8 into ray-project:master Jun 2, 2018

robertnishihara deleted the start-actor-workers-parallel branch June 2, 2018 06:04

robertnishihara mentioned this pull request Aug 27, 2018

Limit number of concurrent workers started by hardware concurrency. #2753

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[xray] Start actor workers in parallel #2168

[xray] Start actor workers in parallel #2168

pcmoritz commented May 30, 2018

robertnishihara May 30, 2018

robertnishihara May 30, 2018

AmplabJenkins commented May 31, 2018

AmplabJenkins commented May 31, 2018

AmplabJenkins commented May 31, 2018

pcmoritz commented May 31, 2018 •

edited

Loading

AmplabJenkins commented May 31, 2018

AmplabJenkins commented May 31, 2018

robertnishihara May 31, 2018

AmplabJenkins commented May 31, 2018

AmplabJenkins commented May 31, 2018

AmplabJenkins commented May 31, 2018

pcmoritz commented May 31, 2018

AmplabJenkins commented May 31, 2018

stephanie-wang left a comment

stephanie-wang Jun 1, 2018

pcmoritz Jun 1, 2018

stephanie-wang Jun 1, 2018

robertnishihara Jun 1, 2018

stephanie-wang Jun 1, 2018

stephanie-wang Jun 1, 2018

robertnishihara Jun 1, 2018

AmplabJenkins commented Jun 1, 2018

AmplabJenkins commented Jun 1, 2018

pcmoritz commented Jun 1, 2018

AmplabJenkins commented Jun 1, 2018

pcmoritz commented Jun 1, 2018

AmplabJenkins commented Jun 2, 2018

[xray] Start actor workers in parallel #2168

[xray] Start actor workers in parallel #2168

Conversation

pcmoritz commented May 30, 2018

What do these changes do?

Related issue number

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented May 31, 2018

AmplabJenkins commented May 31, 2018

AmplabJenkins commented May 31, 2018

pcmoritz commented May 31, 2018 • edited Loading

AmplabJenkins commented May 31, 2018

AmplabJenkins commented May 31, 2018

Choose a reason for hiding this comment

AmplabJenkins commented May 31, 2018

AmplabJenkins commented May 31, 2018

AmplabJenkins commented May 31, 2018

pcmoritz commented May 31, 2018

AmplabJenkins commented May 31, 2018

stephanie-wang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Jun 1, 2018

AmplabJenkins commented Jun 1, 2018

pcmoritz commented Jun 1, 2018

AmplabJenkins commented Jun 1, 2018

pcmoritz commented Jun 1, 2018

AmplabJenkins commented Jun 2, 2018

pcmoritz commented May 31, 2018 •

edited

Loading