Refine multi-threading support #3672

raulchen · 2019-01-02T03:31:27Z

What do these changes do?

Make current_task_id, task_index, and put_index thread local.
For main thread, current_task_id is the real task id, which gets set/reset before/after each task. For background threads, current_task_id is a fake random id that will be generated when it's used for the first time, and it will never change again.
task_index and put_index still increments monotonically each time when submit_task and put is called (so task id generation is still deterministic for the main thread).
state_lock isn't needed anymore (could improve efficiency).
task_driver_id is still shared among threads. But we don't reset it to NIL for actors.
fix issue of calling an actor from multiple threads.

Related issue number

AmplabJenkins · 2019-01-02T04:53:53Z

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

AmplabJenkins · 2019-01-02T10:42:23Z

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

AmplabJenkins · 2019-01-02T12:48:47Z

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

stephanie-wang · 2019-01-03T00:40:17Z

java/runtime/src/main/java/org/ray/runtime/AbstractRayRuntime.java

@@ -71,14 +71,14 @@ public AbstractRayRuntime(RayConfig rayConfig) {
  @Override
  public <T> RayObject<T> put(T obj) {
    UniqueId objectId = UniqueIdUtil.computePutId(
-        workerContext.getCurrentTask().taskId, workerContext.nextPutIndex());
+        workerContext.getCurrentTaskId(), workerContext.nextPutIndex());


Hmm, I just realized that this may interact poorly with fault tolerance. If an object created on a separate thread is lost, then the backend will probably crash because the object won't have any lineage. We don't need to address it in this PR, but just something to keep in mind...

Good point. Maybe we should change this check to a warning?

Yeah, eventually we probably should, but it's still useful right now for debugging purposes.

stephanie-wang · 2019-01-03T00:42:05Z

java/runtime/src/main/java/org/ray/runtime/WorkerContext.java

-    currentTaskCallCount = new AtomicInteger(0);
-    currentClassLoader = null;
-    currentTask = createDummyTask(workerMode, driverId);
+    currentTaskId = ThreadLocal.withInitial(UniqueId::randomId);


Should this be initialized with nil? It looks like it gets overwritten soon after in both branches if the if-block below.

ThreadLocal.withInitial takes a factory method as the parameter. It means that if no value has been set for the current thread, the factory method will be called and set the return value as the thread local value.
So here, the factory method is a method that generates a random ID. A few lines below, we call currentTaskId.set. This set the value only for the main thread (a random ID for driver, and NIL for worker).
Then afterwards, when we call currentTaskId.get in a background thread for the first time. The factory method will be called, and set value for the thread.

I see, thanks!

stephanie-wang · 2019-01-03T00:48:25Z

java/runtime/src/main/java/org/ray/runtime/WorkerContext.java

-    Preconditions.checkState(!taskId.isNil());
-    return taskId;
+  public UniqueId getCurrentTaskId() {
+    return currentTaskId.get();


I think I'm probably missing something about Java, but how exactly does currentTaskId get set to a random ID for new threads?

stephanie-wang · 2019-01-03T00:54:59Z

python/ray/worker.py

+TaskContext = namedtuple('TaskContext', [
+    'current_task_id',
+    'task_index',
+    'put_index',


This should probably also include the current task's driver ID since this changes for non-actors.

task_driver_id isn't thread-specific. At any moment, all threads should have the same driver id. So it shouldn't be here. Does that make sense?

Ah, I see. I was thinking of the case where a thread started during a task is still active after the worker finishes the task. Ideally, we would just prohibit that case, but not sure it's possible.

We don't proactively check such threads after a non-actor task finishes. But if the thread continues submitting tasks while there's no running task in the main thread, the following check will fail.
assert self.task_driver_id.is_nil()

stephanie-wang · 2019-01-03T00:55:35Z

python/ray/worker.py

+
+            self._task_context.task_index = 0
+            self._task_context.put_index = 1
+            self._task_context.initialized = True


This doesn't actually seem to use TaskContext. Is that intentional?

I don't understand what you mean by use TaskContext

I mean that self._task_context is not actually an instance of TaskContext.

ah, I see. good catch.

AmplabJenkins · 2019-01-03T10:33:12Z

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

AmplabJenkins · 2019-01-04T08:58:55Z

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

AmplabJenkins · 2019-01-09T08:51:53Z

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

stephanie-wang · 2019-01-09T20:30:50Z

Looks good! Can you resolve the conflicts, and then we can merge?

AmplabJenkins · 2019-01-10T05:30:07Z

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

fix

fix java

AmplabJenkins · 2019-01-10T09:37:54Z

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

AmplabJenkins · 2019-01-10T14:21:27Z

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

robertnishihara · 2019-01-11T22:17:53Z

python/ray/actor.py

-        self._ray_actor_cursor = object_ids.pop()
-        # We have notified the backend of the new actor handles to expect since
-        # the last task was submitted, so clear the list.
-        self._ray_new_actor_handles = []


Was the deletion of this line self._ray_new_actor_handles = [] intentional or an accident?

@raulchen @stephanie-wang

I think this was an accident, my bad!

my bad, I'll submit an PR to fix this soon.

robertnishihara · 2019-01-11T22:32:28Z

@raulchen I think this PR introduced #3754. Can you take a look?

raulchen force-pushed the multi_thread branch from b6236cf to 0d37ba4 Compare January 2, 2019 09:19

raulchen changed the title ~~[WIP] Refine multi-threading support~~ Refine multi-threading support Jan 2, 2019

raulchen requested review from robertnishihara, stephanie-wang and guoyuhong January 2, 2019 09:32

stephanie-wang reviewed Jan 3, 2019

View reviewed changes

raulchen force-pushed the multi_thread branch from 8591e96 to b7ee9f5 Compare January 3, 2019 10:07

raulchen force-pushed the multi_thread branch from 085dd3c to 0082bde Compare January 9, 2019 06:34

raulchen force-pushed the multi_thread branch from 0082bde to 09066ca Compare January 10, 2019 04:21

raulchen added 2 commits January 10, 2019 14:31

[Python] refine multi-threading support

c65debd

fix

[java] refine multithreading code

ef4d465

fix java

raulchen force-pushed the multi_thread branch from 09066ca to ef4d465 Compare January 10, 2019 06:31

format

55bf153

stephanie-wang approved these changes Jan 10, 2019

View reviewed changes

stephanie-wang merged commit 597abb2 into ray-project:master Jan 10, 2019

raulchen mentioned this pull request Jan 11, 2019

Issue when calling a remote function in a background thread #3651

Closed

robertnishihara reviewed Jan 11, 2019

View reviewed changes

raulchen mentioned this pull request Jan 12, 2019

Fix new handles not reset after actor task submission #3759

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refine multi-threading support #3672

Refine multi-threading support #3672

raulchen commented Jan 2, 2019 •

edited

Loading

AmplabJenkins commented Jan 2, 2019

AmplabJenkins commented Jan 2, 2019

AmplabJenkins commented Jan 2, 2019

stephanie-wang Jan 3, 2019

raulchen Jan 3, 2019

stephanie-wang Jan 9, 2019

stephanie-wang Jan 3, 2019

raulchen Jan 3, 2019

stephanie-wang Jan 9, 2019

stephanie-wang Jan 3, 2019

stephanie-wang Jan 3, 2019

raulchen Jan 3, 2019

stephanie-wang Jan 9, 2019

raulchen Jan 10, 2019

stephanie-wang Jan 3, 2019

raulchen Jan 3, 2019

stephanie-wang Jan 9, 2019

raulchen Jan 10, 2019

AmplabJenkins commented Jan 3, 2019

AmplabJenkins commented Jan 4, 2019

AmplabJenkins commented Jan 9, 2019

stephanie-wang commented Jan 9, 2019

AmplabJenkins commented Jan 10, 2019

AmplabJenkins commented Jan 10, 2019

AmplabJenkins commented Jan 10, 2019

robertnishihara Jan 11, 2019

stephanie-wang Jan 12, 2019

raulchen Jan 12, 2019

raulchen Jan 12, 2019

robertnishihara commented Jan 11, 2019

Refine multi-threading support #3672

Refine multi-threading support #3672

Conversation

raulchen commented Jan 2, 2019 • edited Loading

What do these changes do?

Related issue number

AmplabJenkins commented Jan 2, 2019

AmplabJenkins commented Jan 2, 2019

AmplabJenkins commented Jan 2, 2019

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

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 Jan 3, 2019

AmplabJenkins commented Jan 4, 2019

AmplabJenkins commented Jan 9, 2019

stephanie-wang commented Jan 9, 2019

AmplabJenkins commented Jan 10, 2019

AmplabJenkins commented Jan 10, 2019

AmplabJenkins commented Jan 10, 2019

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

robertnishihara commented Jan 11, 2019

raulchen commented Jan 2, 2019 •

edited

Loading