support multiple workers in the same process #121

CodingCat · 2017-12-01T07:59:17Z

No description provided.

CodingCat · 2017-12-01T23:08:53Z

yzhliu

We need to track customer_id in the scheduler (for detecting nodes from the same ip:port), that's why customer_id is recorded everywhere. Am I right?

yzhliu · 2017-12-06T06:35:56Z

include/ps/internal/customer.h

-  void Accept(const Message& recved) { recv_queue_.Push(recved); }
+  void Accept(const Message& recved) {
+    recv_queue_.Push(recved);
+  }


better to make these functions inline
(app_id(), customer_id(), Accept())

yzhliu · 2017-12-06T06:39:33Z

include/ps/internal/van.h

+    /**
+     * \brief whether it is ready for sending. thread safe
+     */
+    bool IsReady() { return ready_; }


inline my_node, GetTimestamp, IsReady

yzhliu · 2017-12-06T06:49:08Z

include/ps/internal/van.h

+    void ProcessDataMsg(Message* msg);
+
+    void UpdateLocalID(Message* msg, std::unordered_set<int>* deadnodes_set, Meta* nodes,
+                       Meta* recovery_nodes);


Pls add comments for these functions.

yzhliu · 2017-12-06T07:05:25Z

include/ps/internal/customer.h

   * \param recv_handle the functino for processing a received message
   */
-  Customer(int id, const RecvHandle& recv_handle);
+  Customer(int app_id, int customer_id, const RecvHandle& recv_handle);


doc for customer_id

yzhliu · 2017-12-06T07:12:05Z

include/ps/internal/postoffice.h

  bool is_worker_, is_server_, is_scheduler_;
  int num_servers_, num_workers_;
-  bool barrier_done_;
+  std::unordered_map<int, std::unordered_map<int, bool>> barrier_done_;


better to insert space between two >, i.e., > >.

yzhliu · 2017-12-06T07:56:41Z

include/ps/internal/van.h

+    int drop_rate_ = 0;
+    std::atomic<int> timestamp_{0};
+    std::mutex start_mu_;
+    int init_stage = 0;


init_stage_
is it the variable to deal with multiple instances starting in one process?

yzhliu · 2017-12-06T08:00:56Z

src/postoffice.cc

-  customers_[id] = customer;
+  int app_id = CHECK_NOTNULL(customer)->app_id();
+  if (customers_.find(app_id) == customers_.end()) {
+    customers_[app_id] = *(new std::unordered_map<int, Customer*>());


memory leak

how? please forgive that I have nearly forgotten everything of c++, but this should be alive as long as the whole process

customers_[...] = will call copy constructor of the right one, i.e., *(new ...). Then you just lost the pointer and have no chance to free the memory.

if I understand correctly, it is because of the [] operator of unordered_map: http://www.cplusplus.com/reference/unordered_map/unordered_map/operator[]/? (If k does not match the key of any element in the container, the function inserts a new element with that key and returns a reference to its mapped value. Notice that this always increases the container size by one, even if no mapped value is assigned to the element (the element is constructed using its default constructor).)

56c90af should fix it

yzhliu · 2017-12-06T08:01:07Z

src/postoffice.cc

+    << customer_id << " already exists\n";
+  customers_[app_id][customer_id] = customer;
+  if (barrier_done_.find(app_id) == barrier_done_.end()) {
+    barrier_done_[app_id] = *(new std::unordered_map<int, bool>());


memory leak

how? please forgive that I have nearly forgotten everything of c++, but this should be alive as long as the whole process

yzhliu · 2017-12-06T08:10:42Z

src/van.cc

+            new std::thread(&Van::Receiving, this));
+    init_stage++;
+  }
+  std::this_thread::sleep_for(std::chrono::milliseconds(3000));


why sleep here?

looks like some leftover after some changes, removed

yzhliu · 2017-12-06T08:21:44Z

tests/test_simple_app.cc

  app.set_request_handle(ReqHandle);

-  Start();
+  Start(0);


since it is always 0 for ps::Start(), do we have to add arg customer_id for it?

it's just a test file, in the following PR (against mxnet, this will be different numbers)

CodingCat · 2017-12-07T04:15:16Z

We need to track customer_id in the scheduler (for detecting nodes from the same ip:port), that's why customer_id is recorded everywhere. Am I right?

yeah, that's part of the reason, we track customer_id everywhere because multiple customers would share the same I/O channel (ip_addr:port) and we need to dispatch the msg to the right thread running the customer

CodingCat · 2017-12-07T04:15:47Z

@Javelinjs thanks for the review, I addressed some and replied in the comments

CodingCat · 2017-12-11T15:17:41Z

@Javelinjs more comments about this?

yzhliu · 2017-12-11T22:30:47Z

good to me @mli

CodingCat · 2017-12-12T17:42:35Z

thank @Javelinjs , ping @mli

mli · 2017-12-18T17:41:42Z

include/ps/internal/van.h

+
+    // node's address string (i.e. ip:port) -> node id
+    // this map is updated when ip:port is received for the first time
+    std::unordered_map<std::string, int> connected_nodes;


add '_' at the end for private variables

mli · 2017-12-18T17:47:47Z

include/ps/simple_app.h

  msg.meta.head = req_head;
  if (req_body.size()) msg.meta.body = req_body;
-  int ts = obj_->NewRequest(recv_id);
+  int ts = obj_ -> NewRequest(recv_id);


no space around ->

mli · 2017-12-18T17:53:25Z

Is there a test case to test running multiple workers in a single process

CodingCat · 2017-12-18T17:54:52Z

@mli , no test case here, but I have tested with mxnet-spark (modified version)

will add one here

CodingCat · 2017-12-19T06:29:06Z

looks like the behavior of the program is quite different in Mac and Travis...I will look into it further

CodingCat · 2017-12-21T04:09:27Z

please do not merge for now, there is some race condition which I am still looking into

CodingCat · 2017-12-24T17:40:44Z

@mli @Javelinjs thanks for the review

it turns out that there is an issue when connecting to the client socket creator for more than one time, I fixed the duplicate connections and repeated tests for many times (with the newly added multi-worker tests)

now it is ready for the further review

Happy Christmas!

CodingCat · 2018-01-02T03:30:37Z

@mli @Javelinjs ping ....

CodingCat · 2018-01-03T03:17:08Z

thanks for the review and merging, will move forward to submit a patch in mxnet repo this week @Javelinjs @mli

CodingCat added 6 commits September 3, 2017 23:05

refactor implementation of van

bd3862e

refactor to make lint happy

9f583dd

lint is picky

fa61262

multiple workers

48f3795

support multiple workers per process

d8a0dee

remove cout

e194d5b

yzhliu reviewed Dec 6, 2017

View reviewed changes

CodingCat added 2 commits December 6, 2017 20:20

address the comments

e6c0680

fix memory leak

56c90af

mli reviewed Dec 18, 2017

View reviewed changes

CodingCat force-pushed the unique_id_2 branch 2 times, most recently from a856a8c to f7bb369 Compare December 24, 2017 17:08

CodingCat added 3 commits December 24, 2017 09:16

add test case with multiworkers

6130c49

remove std::cout

f7bb369

stylistic fix

b83e24f

recover other tests

be0d918

mli merged commit aee3252 into dmlc:master Jan 2, 2018

CodingCat mentioned this pull request Jan 12, 2018

[SUBMODULE] Upgrade ps-lite to the latest version apache/mxnet#9389

Merged

5 tasks

support multiple workers in the same process #121

support multiple workers in the same process #121

Uh oh!

Conversation

CodingCat commented Dec 1, 2017

Uh oh!

CodingCat commented Dec 1, 2017

Uh oh!

yzhliu left a comment

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!

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!

CodingCat commented Dec 7, 2017

Uh oh!

CodingCat commented Dec 7, 2017

Uh oh!

CodingCat commented Dec 11, 2017

Uh oh!

yzhliu commented Dec 11, 2017

Uh oh!

CodingCat commented Dec 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mli commented Dec 18, 2017

Uh oh!

CodingCat commented Dec 18, 2017

Uh oh!

CodingCat commented Dec 19, 2017

Uh oh!

CodingCat commented Dec 21, 2017

Uh oh!

CodingCat commented Dec 24, 2017

Uh oh!

CodingCat commented Jan 2, 2018