Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding an initial implementation for the unbuffered channel #7984

Merged
merged 11 commits into from
Feb 1, 2018

Conversation

abhinavarora
Copy link
Contributor

@abhinavarora abhinavarora commented Jan 30, 2018

@kavyasrinet and I worked together on designing this initial code for Unbuffered Channels

@kavyasrinet
Copy link

As of now, the code proposed in this PR builds and compiles. We are working on coming up with a basic test case to test the functionality before we put it up for review.

});
int recv;
ch->Receive(&recv);
EXPECT_EQ(recv, 25U);
Copy link
Contributor

Choose a reason for hiding this comment

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

To ensure that UnbufferedChannel works well, I think that we should call Send and Receive many times.
e.g.

  size_t sum_send = 0;
  std::thread t([&]() {
    for (size_t i = 1; i <= 3; ++i) {
      ch->Send(&i);
      sum_send += i;
    }
  });
  size_t sum_rece = 0;
  size_t recv;
  for (size_t i = i; i <= 3; ++i) {
    EXPECT_EQ(sum_send, sum_rece);
    ch-> Receive(&recv);
    EXPECT_EQ(recv, i);
    sum_rece += i;
    std::this_thread::sleep_for(std::chrono::milliseconds(100));
  }

Choose a reason for hiding this comment

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

Thanks so much for the feedback! We will be definitely adding more test cases to this file like the one you mentioned here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chengduoZH Thank you for the feedback. Fixed it!

@abhinavarora
Copy link
Contributor Author

This PR only contains a simple unit test. We will add more unit tests after the PR is merged.

Copy link
Contributor

@chengduoZH chengduoZH left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@wangkuiyi wangkuiyi left a comment

Choose a reason for hiding this comment

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

It seems that this PR is in its very early stage -- we need many more unit tests to make sure that the implementation works.

CloseChannel(ch);
t.join();
EXPECT_EQ(sum_send, 10U);
delete ch;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please make sure that the destructor calls CoseChannel if necessary, so that users can do delete without calling CloseChannel in prior.

@abhinavarora abhinavarora merged commit 6d8bc13 into PaddlePaddle:develop Feb 1, 2018
@abhinavarora abhinavarora deleted the unbuffered_channel branch February 1, 2018 22:33
@kavyasrinet
Copy link

Yes, we will be adding more unit tests in successive PRs as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants