-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Adding an initial implementation for the unbuffered channel #7984
Conversation
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. |
…nto unbuffered_channel
paddle/framework/channel_test.cc
Outdated
}); | ||
int recv; | ||
ch->Receive(&recv); | ||
EXPECT_EQ(recv, 25U); |
There was a problem hiding this comment.
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));
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
This PR only contains a simple unit test. We will add more unit tests after the PR is merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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; |
There was a problem hiding this comment.
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.
Yes, we will be adding more unit tests in successive PRs as well. |
@kavyasrinet and I worked together on designing this initial code for Unbuffered Channels