Implement local scheduler task queues using C++ data structures by stephanie-wang · Pull Request #392 · ray-project/ray

stephanie-wang · 2017-03-24T02:45:03Z

This replaces the utarray and utlist C library implementation for the local scheduler's task queue with an implementation using the C++ STL.

robertnishihara · 2017-03-24T02:50:07Z

This is related to #324 (which does something similar for Plasma), though #324 needs to be rebased.

robertnishihara · 2017-03-24T02:57:04Z

FYI @rshin.

AmplabJenkins · 2017-03-24T03:05:28Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-03-24T03:05:29Z

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

rshin · 2017-03-24T04:22:32Z

 void remove_actor(SchedulingAlgorithmState *algorithm_state, ActorID actor_id);

-typedef struct task_queue_entry {
+typedef struct TaskQueueEntry {


typedef is unnecessary in C++

rshin · 2017-03-24T04:24:00Z

  object_entry *remote_objects;
 };

+TaskQueueEntry *TaskQueueEntry_init(TaskSpec *spec, int64_t task_spec_size) {


You might want to replace these with a constructor and a destructor.

rshin · 2017-03-24T04:25:32Z

  /** A queue of tasks to be executed on this actor. The tasks will be sorted by
   *  the order of their actor counters. */
-  task_queue_entry *task_queue;
+  std::list<TaskQueueEntry *> *task_queue;


If you drop the *s (std::list<TaskQueueEntry> task_queue) then you don't need the allocation/deallocation code.

You should probably use std::vector instead of std::list as it has better data locality

I think it may actually make more sense to use std::list (or maybe a deque) in this case, since we remove elements from the head and occasionally the middle.

rshin · 2017-03-24T04:28:16Z

-    free(elt->spec);
-    free(elt);
+  for (auto it = algorithm_state->dispatch_task_queue->begin();
+       it != algorithm_state->dispatch_task_queue->end(); ++it) {


Use for (auto &entry : algorithm_state->dispatch_task_queue) {
(but this can also be avoided by not putting pointers in the list)

wesm · 2017-03-24T15:32:33Z

  /** A queue of tasks to be executed on this actor. The tasks will be sorted by
   *  the order of their actor counters. */
-  task_queue_entry *task_queue;
+  std::list<TaskQueueEntry *> *task_queue;


You should probably use std::vector instead of std::list as it has better data locality

wesm · 2017-03-24T15:35:08Z

  HASH_ITER(hh, algorithm_state->remote_objects, obj_entry, tmp_obj_entry) {
    HASH_DELETE(hh, algorithm_state->remote_objects, obj_entry);
-    utarray_free(obj_entry->dependent_tasks);
+    delete obj_entry->dependent_tasks;


In modern C++ code, it is good idea to try to never use new or delete. It is better to use std::unique_ptr or some other smart pointer to handle deallocation in an error-safe way (this is usually called RAII)

wesm · 2017-03-24T15:35:35Z

  entry->task_counter = 0;
  /* Initialize the doubly-linked list to NULL. */
-  entry->task_queue = NULL;
+  entry->task_queue = new std::list<TaskQueueEntry *>();


See comments above. Perhaps the task queue would be better encapsulated in a class?

robertnishihara · 2017-03-24T21:35:34Z

Thanks @rshin and @wesm! Those are good points. And I agree it'd be nice to find a way to avoid the std::list.

@stephanie-wang, I guess the question is should we try to convert from C to C++ gradually? Or should we just go ahead and convert the whole local scheduler right now (e.g., switch to using classes, no calls to new/delete etc.

Do you have a sense of how long that conversion would take?

AmplabJenkins · 2017-03-28T00:15:03Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-03-28T00:15: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/400/
Test PASSed.

robertnishihara · 2017-03-28T07:50:18Z

should we also do free(entry) here?

Since we're using std::list<TaskQueueEntry>, entries that are removed from the list will automatically get freed (in contrast to if we had a std::list<TaskQueueEntry *> like in a previous commit).

robertnishihara · 2017-03-28T07:52:58Z

Can we do this? We just memset algorithm_state to all 0's and now we're calling a method of one of the fields?

This has to do with the way that std::unique_ptr's work. I think that zeroing out a std::unique_ptr is like initializing it to hold a NULL pointer. You can then replace the pointer that it holds with a pointer to an actual object, like we're doing here.

These docs were useful: http://www.cplusplus.com/reference/memory/unique_ptr/

I haven't seen the whole code, but generally speaking you should (almost) never need std::unique_ptr<std::vector<T> >; you should just do std::vector<T> and then call vec1.swap(vec2) if you need to move them around the vectors for some reason. Note that swap() doesn't invalid iterators/pointers/references to elements for std::vector, so it's not a problem if that's what you're worried about (this behavior differs from that of std::basic_string).

Oh and the same goes for std::unique_ptr<std::list<T> >.

robertnishihara · 2017-03-28T07:55:33Z

Why did you introduce the calls to memset? I imagine it crashes without the memset?

Yeah, this is a bit awkward, but it has to be this way for now while we're mixing C structs and C++ objects. When we malloc the LocalActorInfo, it might get initialized with garbage values. A std::unique_ptr that is a member of the struct will also get initialized with garbage values, and it will seem to hold a pointer, but the pointer isn't valid. When we reset the std::unique_ptr to point to the actual object we want it to point to, it tries to clean up the invalid pointer under the hood. So, I used memset so that it gets initialized to hold a NULL pointer.

The (semi-)C++ way to do this would be LocalActorInfo *entry = new LocalActorInfo(), and to then define LocalActorInfo::LocalActorInfo() : member1(), member2(), member3(), ... { }, and finally call delete after. As-is, this is a bad idea because if LocalActorInfo ever ends up with anything that has a destructor, you'll either have to call them manually (at which point you might as well define the destructor and just call delete) or they won't be called at all.

(The full C++ way would use something like std::unique_ptr etc. which I'd recommend gradually switching wherever you're finding yourself making other changes like these.)

mehrdadn · 2017-03-28T08:34:11Z

I'd just use size_t (or maybe auto, though I'd be careful where I sprinkle auto) here. No need to change the signedness unnecessarily.

mehrdadn · 2017-03-28T08:36:29Z

You should never have to explicitly free resources anywhere but inside a destructor in C++ code.
It seems like what you really want is std::list<std::unique_ptr<TaskQueueEntry> >, with the destructor TaskQueueEntry::~TaskQueueEntry defined appropriately. Then you can take out this for loop and just call algorithm_state->waiting_task_queue.reset() if needed.

(Or maybe just std::list<TaskQueueEntry> -- depends on what you're trying to do.)

mehrdadn · 2017-03-28T08:40:10Z

C++ way would be to just define the constructor TaskQueueEntry::TaskQueueEntry appropriately; no init methods anywhere.

mehrdadn · 2017-03-28T08:42:30Z

C++ way would be to express the algorithm at a higher level like this:

std::find_if(entry->task_queue->begin(), entry->task_queue->end(), [&](TaskQueueEntry &v) { return task_counter > TaskSpec_actor_counter(v); })

atumanov · 2017-03-28T08:49:14Z

Let's make a sanity check here : is this the right time to do this C to C++ conversion?

mehrdadn · 2017-03-28T08:50:29Z

Higher-level comment (not C++-related, maybe for later), but a linear scan seems like something you should never want to do. Perhaps a priority queue?

mehrdadn · 2017-03-28T08:52:42Z

It seems you don't need to copy this? You can just use it directly right?

mehrdadn · 2017-03-28T08:53:39Z

Never use int to denote a size. :)

mehrdadn · 2017-03-28T08:59:00Z

(P.S. sorry, I just saw some of my comments were repeats of earlier ones by others... didn't see that until now.)

robertnishihara · 2017-03-28T20:31:53Z

@mehrdadn thanks for the review! I think this discussion makes it clear that there is a ton of work to do to convert the project to a C++ style #406. It's probably not desirable to radically change the style locally (in this PR) without doing it more globally as well. That is, if we're using malloc/free for memory management everywhere else in the project, and we need to use something in the C++ STL, then it looks like the simplest thing to do is to do a direct translation from malloc/free to new/delete and to defer the usage of unique_ptr a little further down the road when we do a project-wide change.

AmplabJenkins · 2017-03-28T21:31:33Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-03-28T21:31: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/406/
Test PASSed.

robertnishihara · 2017-03-28T23:55:09Z

+    ++it;
  }
-  DL_APPEND_ELEM(entry->task_queue, current_entry, elt);
+  entry->task_queue->insert(it, elt);


This is a change in semantics (insert prepends whereas before we were appending). It looks like the previous approach was incorrect, although maybe the problem never arose because actor methods always arrived in order.

robertnishihara · 2017-03-29T00:04:53Z

-                             TaskSpec *spec,
-                             int64_t task_spec_size,
-                             bool from_global_scheduler) {
+TaskQueueEntry *queue_task(LocalSchedulerState *state,


should return Void.

AmplabJenkins · 2017-03-29T08:41:26Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-03-29T08:41: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/408/
Test PASSed.

pcmoritz · 2017-03-29T15:09:08Z

-  task_queue_entry *elt;
-  int count;
-  DL_COUNT(algorithm_state->dispatch_task_queue, elt, count);
+  TaskQueueEntry *elt;


can get rid of this, maybe get rid of the whole method now

pcmoritz · 2017-03-29T15:09:44Z

-  task_queue_entry *elt;
-  int count;
-  DL_COUNT(algorithm_state->waiting_task_queue, elt, count);
+  TaskQueueEntry *elt;


can get rid of this, maybe can get rid of the whole method now

Hmm I think we still need it for testing purposes (definition of SchedulingAlgorithmState isn't exposed).

AmplabJenkins · 2017-03-29T17:56:34Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-03-29T17:56:35Z

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

robertnishihara · 2017-03-29T20:42:13Z

-  /** A vector of the tasks dependent on this object. */
-  std::vector<TaskQueueEntry> *dependent_tasks;
+  /** A vector of tasks dependent on this object. These tasks are a subset of
+   * the tasks in the waiting queue. Each element is actually store a reference


these lines should have two spaces after the *

robertnishihara · 2017-03-29T21:27:18Z

-  fetch_missing_dependencies(state, algorithm_state, &task_entry);
+  /* There must be at least one missing dependency, so record it. The task was
+   * just queued, so we can get a reference to it by going to the last element
+   * in the waiting queue. */


This comment is a bit scary.

Can we avoid this by passing in the iterator?

If we really need to do this, maybe we should do CHECK(it->spec == spec) or something like that.

We have to get the iterator somehow, and I think this is the best way to do it. I changed this so that queue_task returns the correct iterator, though, so we can be sure that no one modified the list in between.

Great, this looks better since it should no longer depend on the implementation of queue_task.

robertnishihara · 2017-03-29T21:46:57Z

+       * spec. */
+      it = algorithm_state->dispatch_task_queue->erase(it);
+    } else {
+      /* The task can still run,so continue to the next task. */


add a space after ,

robertnishihara · 2017-03-29T21:49:09Z

-          DL_DELETE(algorithm_state->dispatch_task_queue, elt);
-          DL_APPEND(algorithm_state->waiting_task_queue, elt);
-          fetch_missing_dependency(state, algorithm_state, elt,
+          fetch_missing_dependency(state, algorithm_state, it,


this doesn't necessary need to be addressed in this PR, but are we potentially calling fetch_missing_dependency a bunch of times on the same object ID?

If so, could you at least add a comment with a TODO to address this?

Yes, we want to do that, since multiple tasks may depend on the same object ID. The same task will never appear more than once under the same object ID though.

AmplabJenkins · 2017-03-29T22:11:21Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-03-29T22:11:22Z

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

AmplabJenkins · 2017-03-29T22:23:11Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-03-29T22:23:12Z

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

AmplabJenkins · 2017-03-29T23:31:37Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-03-29T23:31:38Z

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

atumanov · 2017-03-29T23:44:40Z

-  /** An array of the tasks dependent on this object. */
-  UT_array *dependent_tasks;
+  /** A vector of tasks dependent on this object. These tasks are a subset of
+   *  the tasks in the waiting queue. Each element is actually store a


nit: Each element actually stores a reference to the corresponding...

atumanov · 2017-03-29T23:49:15Z

  /* Assign as many tasks as we can, while there are workers available. */
-  DL_FOREACH_SAFE(algorithm_state->dispatch_task_queue, elt, tmp) {
+  auto it = algorithm_state->dispatch_task_queue->begin();
+  while (it != algorithm_state->dispatch_task_queue->end()) {


this could be written as a for loop:
for (auto it = algorithm_state->dispatch_task_queue->begin(); it != algorithm_state->dispatch_task_queue->end();) {

AmplabJenkins · 2017-03-30T07:13:51Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-03-30T07:13:51Z

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

…project#392) * Switch to using C++ lists for task queues * Init and free methods for TaskQueueEntry * Switch from utarray to c++ vector for TaskQueueEntry * Get rid of some pointers * Back to O(1) deletion from waiting_task_queue * Fix comments * Cut code * Non const iterators * Fix Alexey's comments

robertnishihara · 2017-04-09T23:37:48Z

+  task_queue->push_back(*task_entry);
+  /* Since we just queued the task, we can get a reference to it by going to
+   * the last element in the queue. */
+  auto it = state->algorithm_state->waiting_task_queue->end();


This looks like a bug, it should be

auto it = task_queue->end();

Is that right @stephanie-wang?

This should be fixed in #443.

Ah, yes. Good catch!

stephanie-wang added 3 commits March 23, 2017 18:48

Switch to using C++ lists for task queues

36ae2a5

Init and free methods for TaskQueueEntry

4d9919e

Switch from utarray to c++ vector for TaskQueueEntry

06bfd36

rshin reviewed Mar 24, 2017

View reviewed changes

wesm reviewed Mar 24, 2017

View reviewed changes

Get rid of some pointers

143d42c

robertnishihara mentioned this pull request Mar 28, 2017

Replace UT data structures with C++ STL. #404

Closed

robertnishihara reviewed Mar 28, 2017

View reviewed changes

mehrdadn reviewed Mar 28, 2017

View reviewed changes

Comment thread src/local_scheduler/local_scheduler_algorithm.cc Outdated

Copy link
Copy Markdown

Contributor

mehrdadn Mar 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never use int to denote a size. :)

stephanie-wang force-pushed the cpp-task-queues branch from 69622ee to 143d42c Compare March 28, 2017 21:15

robertnishihara reviewed Mar 29, 2017

View reviewed changes

pcmoritz reviewed Mar 29, 2017

View reviewed changes

Back to O(1) deletion from waiting_task_queue

ebea807

robertnishihara reviewed Mar 29, 2017

View reviewed changes

stephanie-wang force-pushed the cpp-task-queues branch from d99e0a4 to 036bdc4 Compare March 29, 2017 22:01

Cut code

4439669

atumanov reviewed Mar 29, 2017

View reviewed changes

stephanie and others added 2 commits March 29, 2017 18:42

Non const iterators

3619f44

Fix comments

036bdc4

robertnishihara merged commit 036b873 into ray-project:master Mar 30, 2017

robertnishihara deleted the cpp-task-queues branch March 30, 2017 07:40

Fix Alexey's comments

6bef865

robertnishihara reviewed Apr 9, 2017

View reviewed changes

Conversation

stephanie-wang commented Mar 24, 2017

Uh oh!

robertnishihara commented Mar 24, 2017

Uh oh!

robertnishihara commented Mar 24, 2017

Uh oh!

AmplabJenkins commented Mar 24, 2017

Uh oh!

AmplabJenkins commented Mar 24, 2017

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!

robertnishihara commented Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AmplabJenkins commented Mar 28, 2017

Uh oh!

AmplabJenkins commented Mar 28, 2017

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!

mehrdadn Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mehrdadn Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atumanov commented Mar 28, 2017

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!

mehrdadn commented Mar 28, 2017

Uh oh!

robertnishihara commented Mar 28, 2017

Uh oh!

robertnishihara commented Mar 24, 2017 •

edited

Loading

mehrdadn Mar 28, 2017 •

edited

Loading

mehrdadn Mar 28, 2017 •

edited

Loading