[Core worker] Serialize ActorHandle in core worker. Make ActorHandle thread safe. #5034

kfstorm · 2019-06-25T17:08:36Z

What do these changes do?

Previously actor handle serialization is implemented in both Java and Python. Now we want to do it in core worker. And in the future when submitting a task, we can pass an actor handle of any language as a task argument to any worker, regardless the language of the worker.

This PR also added a mutex for ActorHandle to ensure critical paths thread safe.

Related issue number

#4850

Linter

I've run scripts/format.sh to lint the changes in this PR.

AmplabJenkins · 2019-06-25T20:13: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/14863/
Test FAILed.

ericl · 2019-06-26T01:02:45Z

src/ray/core_worker/task_interface.h

  ObjectID actor_cursor_;
-  /// Counter for tasks from this handle.
+  /// The number of tasks that have been invoked on this actor.


Add comment that task_counter, num_forks, new_actor_handles are guarded by the mutex?

ericl · 2019-06-26T01:05:40Z

src/ray/core_worker/task_interface.h

+        actor_id_, ComputeNextActorHandleId(actor_handle_id_, ++num_forks_),
+        actor_language_, actor_definition_descriptor_));
+    new_handle->actor_cursor_ = actor_cursor_;
+    new_actor_handles_.push_back(new_handle->actor_handle_id_);


I'm a bit confused about the lifecycle of the new actor handles. Why are the forked handles kept in a list until the next actor task before they are cleared?

@raulchen Can you give a comment?

It's used for garbage-collecting the dummy objects. Because we need to know wether the current dummy object is depended by others. @stephanie-wang implemented this in #3593

ericl · 2019-06-26T01:06:08Z

src/ray/protobuf/core_worker.proto

+  bytes actor_cursor = 5;
+
+  // The number of tasks that have been invoked on this actor.
+  int32 task_counter = 6;


ericl · 2019-06-26T01:06:11Z

src/ray/protobuf/core_worker.proto

+
+  // The number of times that this actor handle has been forked.
+  // It's used to make sure ids of actor handles are unique.
+  int32 num_forks = 7;


AmplabJenkins · 2019-06-26T06: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/14887/
Test FAILed.

…andle

raulchen · 2019-06-26T07:32:56Z

src/ray/core_worker/task_interface.cc

+    break;
+  case WorkerLanguage::JAVA:
+    RAY_CHECK(function.function_descriptor.size() == 3);
+    actor_definition_descriptor.push_back(function.function_descriptor[0]);


I think it's okay to just save actor creation task's function descriptor in the handle. Then we don't need to introduce this new concept ActorDefinitionDescriptor

Changed to actor_creation_task_function_descriptor

raulchen · 2019-06-26T07:40:44Z

src/ray/core_worker/task_interface.h

+    temp.set_actor_cursor(actor_cursor_.Binary());
+    temp.set_task_counter(task_counter_);
+    temp.set_num_forks(num_forks_);
+    temp.SerializeToString(output);


temp.SerializeToString(output); doesn't need to be locked.

The new code wrapped ray::rpc::ActorHandle in ray::ActorHandle. The comment doesn't apply any more.

raulchen · 2019-06-26T07:41:56Z

src/ray/core_worker/task_interface.h

+        ray::ActorHandleID::FromBinary(temp.actor_handle_id()),
+        (WorkerLanguage)temp.actor_language(), actor_definition_descriptor,
+        ray::ObjectID::FromBinary(temp.actor_cursor()), temp.task_counter(),
+        temp.num_forks()));


Maybe it's easier to just make ActorHandle hold a rpc::ActorHandle?

AmplabJenkins · 2019-06-26T09:35:13Z

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

AmplabJenkins · 2019-06-26T14:37: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/14893/
Test PASSed.

jovany-wang · 2019-06-26T17:05:22Z

src/ray/common/id.cc

+  sha256_init(&ctx);
+  sha256_update(&ctx, reinterpret_cast<const BYTE *>(actor_handle_id.Data()),
+                actor_handle_id.Size());
+  sha256_update(&ctx, (const BYTE *)&num_forks, sizeof(num_forks));


use safety cast ?

jovany-wang · 2019-06-26T17:06:47Z

src/ray/common/id.cc

@@ -129,6 +129,21 @@ const TaskID GenerateTaskId(const DriverID &driver_id, const TaskID &parent_task
  return TaskID::FromBinary(std::string(buff, buff + TaskID::Size()));
 }

+const ActorHandleID ComputeNextActorHandleId(const ActorHandleID &actor_handle_id,


How about putting this method to a separated file id_util.h to keep id.h clean.

I just followed GenerateTaskId. Maybe move it later?

jovany-wang · 2019-06-26T17:09:56Z

src/ray/core_worker/common.h

+    return ::Language::CPP;
+    break;
+  default:
+    RAY_LOG(FATAL) << "invalid language specified: " << static_cast<int>(language);


invalid -> Invalid

We should log the meaningful name instead of an int?

You can't get a meaningful name if the value is out of valid range of enum. e.g. static_cast<ray::rpc::Language>(100). And eventually, the old, flatbuffer generated definition of language will be replaced by protobuf generated one. So these two conversion functions will go away in the near future.

jovany-wang · 2019-06-26T17:13:33Z

src/ray/core_worker/core_worker.h

  /// Type of this worker.
  const enum WorkerType worker_type_;

  /// Language of this worker.
-  const enum WorkerLanguage language_;
+  const enum ray::rpc::Language language_;


remove the enum keyword.

jovany-wang · 2019-06-26T17:15:32Z

src/ray/core_worker/core_worker_test.cc

+  ASSERT_NE(handle1.ActorHandleID(), forkedHandle1->ActorHandleID());
+  ASSERT_EQ(handle1.ActorLanguage(), forkedHandle1->ActorLanguage());
+  RAY_LOG(INFO) << handle1.ActorCreationTaskFunctionDescriptor().size();
+  RAY_LOG(INFO) << forkedHandle1->ActorCreationTaskFunctionDescriptor().size();


Are these logs useful? I guess they could be removed, right?

jovany-wang · 2019-06-26T17:21:11Z

src/ray/core_worker/task_interface.h

+    auto &original = inner_.actor_creation_task_function_descriptor();
+    *new_handle->inner_.mutable_actor_creation_task_function_descriptor() = {
+        original.begin(), original.end()};
+


remove some blank lines?

AmplabJenkins · 2019-06-26T17:42:41Z

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

raulchen · 2019-06-27T07:20:44Z

src/ray/core_worker/task_interface.h

+  const int64_t NumForks() const { return inner_.num_forks(); };
+
+  std::unique_ptr<ActorHandle> Fork() {
+    auto new_handle = std::unique_ptr<ActorHandle>(new ActorHandle());


you can use new_handle.inner_.CopyFrom(inner_), and then only modify the changed fields

I changed it to new_handle.inner_ = inner_.

raulchen · 2019-06-27T07:23:48Z

src/ray/core_worker/task_interface.h

+  /// It's used to make sure ids of actor handles are unique.
+  const int64_t NumForks() const { return inner_.num_forks(); };
+
+  std::unique_ptr<ActorHandle> Fork() {


Why does this return a unique_ptr, instead of just an object?

jovany-wang · 2019-06-27T07:36:30Z

src/ray/core_worker/task_interface.h

+  /// It's used to make sure ids of actor handles are unique.
+  const int64_t NumForks() const { return inner_.num_forks(); };
+
+  std::unique_ptr<ActorHandle> Fork() {


I'm just curious about the reason for not putting the definition of this method to .cc file?

edoakes · 2019-06-27T10:10:10Z

src/ray/core_worker/common.h

+///
+/// \param[in] language Language for a task.
+/// \return Translated task language.
+inline ::Language ToTaskLanguage(ray::rpc::Language language) {


Having the same name for both of these functions might make it harder to read. For clarity, it would be helpful to specify the "direction" of the translation in the function name, like:
::Language ToCoreTaskLanguage(ray::rpc::Language language)
and for the one below:
ray::rpc::Language ToRPCTaskLanguage(::Language language)

AmplabJenkins · 2019-06-27T10:35:04Z

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

AmplabJenkins · 2019-06-27T12:19:01Z

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

…andle

AmplabJenkins · 2019-06-27T13:33:43Z

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

AmplabJenkins · 2019-06-27T14:09:32Z

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

edoakes · 2019-06-27T16:20:58Z

src/ray/core_worker/task_execution.cc

@@ -29,7 +29,7 @@ Status CoreWorkerTaskExecutionInterface::Run(const TaskExecutor &executor) {
      const auto &spec = task.GetTaskSpecification();
      core_worker_.worker_context_.SetCurrentTask(spec);

-      ray::rpc::Language language = ToTaskLanguage(spec.GetLanguage());
+      ray::rpc::Language language = ToRpcTaskLanguage(spec.GetLanguage());


nit: RPC not Rpc
Unless there's a convention of doing pure camel case even for initialisms in Ray - I'm not sure!

I think both naming conventions are being used widely. I googled this a while ago, a more usual practice is to use all upper cases if the the acronym is less than 2 letters (e.g., IO), and use camel case otherwise (e.g., Rpc).
Note, currently we use TaskID in c++ and Python, and TaskId in Java. That is because all Java linters prefer camel case regardless of word length.

I just took a look. Google's C++ style guide suggest Rpc.

For some symbols, this style guide recommends names to start with a capital letter and to have a capital letter for each new word (a.k.a. "Camel Case" or "Pascal case"). When abbreviations or acronyms appear in such names, prefer to capitalize the abbreviations or acronyms as single words (i.e StartRpc(), not StartRPC()).

https://google.github.io/styleguide/cppguide.html#Naming

AmplabJenkins · 2019-06-27T16:23:25Z

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

raulchen · 2019-06-28T03:00:57Z

@kfstorm Could you resolve the conflicts?

…andle

AmplabJenkins · 2019-06-28T09:10:18Z

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

AmplabJenkins · 2019-06-28T11:07:31Z

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

AmplabJenkins · 2019-06-28T14:00:16Z

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

AmplabJenkins · 2019-06-28T15:29:35Z

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

AmplabJenkins · 2019-06-28T16:48:54Z

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

AmplabJenkins · 2019-06-28T19:41:56Z

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

jovany-wang

Thanks! I think this can be merged after addressing these minor comments.

src/ray/core_worker/task_interface.cc

src/ray/core_worker/task_interface.h

AmplabJenkins · 2019-07-01T08:53:04Z

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

jovany-wang · 2019-07-01T11:25:53Z

Linting failed.

AmplabJenkins · 2019-07-01T14:54:12Z

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

AmplabJenkins · 2019-07-01T15:15:43Z

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

AmplabJenkins · 2019-07-02T07:57:06Z

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

Serialize ActorHandle in core worker. Make ActorHandle thread safe.

6c7fee0

ericl self-assigned this Jun 25, 2019

ericl reviewed Jun 26, 2019

View reviewed changes

Address comments

678f9ce

Merge remote-tracking branch 'origin/master' into core_worker_actor_h…

76b3562

…andle

raulchen reviewed Jun 26, 2019

View reviewed changes

kfstorm added 3 commits June 26, 2019 19:01

Address comments

0d82369

Address comments

85b4f09

Address comments

8c20e4c

lint

1a2c5a2

jovany-wang reviewed Jun 26, 2019

View reviewed changes

Address comments

5233862

raulchen reviewed Jun 27, 2019

View reviewed changes

jovany-wang reviewed Jun 27, 2019

View reviewed changes

Address comments

a77121f

raulchen approved these changes Jun 27, 2019

View reviewed changes

Address comments

de35b3e

edoakes reviewed Jun 27, 2019

View reviewed changes

Address comments

78cdcdb

Merge remote-tracking branch 'origin/master' into core_worker_actor_h…

59ddc1d

…andle

edoakes reviewed Jun 27, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into core_worker_actor_h…

6403c94

…andle

Minor update

65aa6ec

kfstorm mentioned this pull request Jun 28, 2019

[WIP] [Java worker] Migrate Java worker on top of C++ core worker #5061

Closed

4 tasks

jovany-wang approved these changes Jun 30, 2019

View reviewed changes

src/ray/core_worker/task_interface.cc Outdated Show resolved Hide resolved

src/ray/core_worker/task_interface.h Outdated Show resolved Hide resolved

Address comments

7205444

raulchen approved these changes Jul 1, 2019

View reviewed changes

lint

cfa0f0d

jovany-wang approved these changes Jul 2, 2019

View reviewed changes

jovany-wang merged commit 1cf7728 into ray-project:master Jul 2, 2019

kfstorm mentioned this pull request Jul 2, 2019

[core worker] Refactor CoreWorker member classes #5062

Merged

1 task

[Core worker] Serialize ActorHandle in core worker. Make ActorHandle thread safe. #5034

[Core worker] Serialize ActorHandle in core worker. Make ActorHandle thread safe. #5034

Uh oh!

Conversation

kfstorm commented Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What do these changes do?

Related issue number

Linter

Uh oh!

AmplabJenkins commented Jun 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Jun 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Jun 26, 2019

Uh oh!

AmplabJenkins commented Jun 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Jun 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfstorm commented Jun 25, 2019 •

edited

Loading