Refine channel test #7946

chengduoZH · 2018-01-29T11:53:56Z

The basic function of buffered_channel.
The function of buffered_channel under multithreading.

typhoonzero · 2018-01-29T11:58:25Z

paddle/framework/channel_test.cc

+ framework::ThreadPool* pool;
+ pool = framework::ThreadPool::GetInstance();
+
+ // Receiver


Can be "Consumer" and "Producer".

typhoonzero · 2018-01-29T12:00:29Z

paddle/framework/channel_test.cc

+ CloseChannel(ch);
+}

+TEST(Channel, Buffered) {


Do we need a test case for "unbuffered" channel too?

The implement of unbuffered channel is empty.

Paddle/paddle/framework/details/unbuffered_channel.h

Lines 41 to 48 in fb1a0df

template <typename T>

void UnBuffered<T>::Send(T* channel_element) {}

template <typename T>

void UnBuffered<T>::Receive(T*) {}

template <typename T>

UnBuffered<T>::~UnBuffered() {}

This issue is about unbuffered channel.

typhoonzero

I see. LGTM!

wangkuiyi · 2018-01-29T18:35:01Z

paddle/framework/channel_test.cc

+ Channel<int>* ch = MakeChannel<int>(capacity);
+
+ framework::ThreadPool* pool;
+ pool = framework::ThreadPool::GetInstance();


For two reasons, let us do not use framework::ThreadPool in this unit test, but use std::thread:

To minimize the dependencies.

framework::ThreadPool sets the number of OS threads automatically, which could be one.

wangkuiyi · 2018-01-29T18:36:25Z

paddle/framework/channel_test.cc

+ pool = framework::ThreadPool::GetInstance();
+
+ // Consumer
+ for (int i = 0; i < capacity; ++i) {


I don't see the purpose of this unit test. Do you have a list of edge cases?

wangkuiyi

Let us remoe the dependency to ThreadPool

wangkuiyi · 2018-01-30T07:47:14Z

paddle/framework/channel.h

+}
+
+template <typename T>
+void DeleteChannel(Channel<T>* ch) {


I agree that we need CloseChannel, which mimics Go's close, and DeleteChannel, which mimics Go's garbage collection.

One step forward, I think we can have only CloseChannel, because DeleteChannel could be simplified as delete ch -- I didn't have to and should not have defined template <typename T> void DeleteChannel.

I agree with you.
We should make a further analysis about CloseChannel. When the channel was closed, the channel_ of Channel may also have some data, I think that receive can also work properly, but send cannot send data to channel_.
So, CloseChannel should not delete Buffered or Unbuffered.

If we close the channel by calling the destructor, the objection will be deleted.
But the destructor also calls notify_one, this causes some data also to be placed in the channel, this is obviously an error.

Paddle/paddle/framework/details/buffered_channel.h

Lines 67 to 72 in 311334e

template <typename T>

Buffered<T>::~Buffered() {

std::unique_lock<std::mutex> lock(mu_);

channel_.clear();

NotifyAllSenders(&lock);

}

I don't want to close the channel via the destructor. My idea is the other way. See #7946 (comment)

wangkuiyi · 2018-01-30T07:53:58Z

paddle/framework/channel_test.cc

 std::this_thread::sleep_for(std::chrono::milliseconds(100)); // wait 0.5 sec
 EXPECT_EQ(sum, 45U);
 CloseChannel(ch);
+ t.join();


It seems that you closed but didn't delete the channel in this test? In my mind, this test should look like

std::unique_ptr<Channel<int>*> ch(MakeChannel<int>(10)); // `delete ch.get()` will be called do_something_here_with(ch); CloseChannel(ch.get());

It seems that you closed but didn't delete the channel in this test?

We can add DeleteChannel behind CloseChannel.

Maye the use of unique_ptr didn't present my idea clearly. Let us use delete explicitly:

auto ch = MakeChannel<int>(10); do_something(ch); CloseChannel(ch); delete ch;

Ok!
If we use delete to destroy the ch but other threads also hold the ch and these threads send data to the ch, there will cause an error.
I wonder that whether this case is existent.

wangkuiyi · 2018-01-30T07:54:42Z

paddle/framework/details/buffered_channel.h

 std::condition_variable empty_cond_var_;
 std::condition_variable full_cond_var_;
 std::deque<T> channel_;
+ bool close;


close => closed_ or closing_

wangkuiyi

LGTM!

Let us merge this PR and go on working on the channels in future PRs.

refine channel test

d992022

chengduoZH requested review from typhoonzero and wangkuiyi January 29, 2018 11:54

typhoonzero reviewed Jan 29, 2018

View reviewed changes

follow comments

75c8c90

typhoonzero previously approved these changes Jan 29, 2018

View reviewed changes

wangkuiyi reviewed Jan 29, 2018

View reviewed changes

wangkuiyi requested changes Jan 29, 2018

View reviewed changes

wangkuiyi added 5 commits January 29, 2018 10:54

Add dependency enforce to threadpool

f9609d2

Revert changes to channel_test.cc

9be862f

Revert changes to channel_test.cc

e9e8344

Add #include "paddle/framework/macros.h"

cf407a5

Add unit tests

62fb71a

wangkuiyi dismissed typhoonzero’s stale review via 62fb71a January 29, 2018 22:06

wangkuiyi previously approved these changes Jan 30, 2018

View reviewed changes

wangkuiyi mentioned this pull request Jan 30, 2018

CI test failed in PyDataProvider2.py #7957

Closed

chengduoZH dismissed wangkuiyi’s stale review via 2b6da3b January 30, 2018 02:10

fix code format

be0525f

chengduoZH force-pushed the feature/Add_channel_test branch 2 times, most recently from ccf829c to be0525f Compare January 30, 2018 03:32

refine close channel

3e7ca5c

chengduoZH force-pushed the feature/Add_channel_test branch from ae1d0f9 to 3e7ca5c Compare January 30, 2018 06:24

wangkuiyi reviewed Jan 30, 2018

View reviewed changes

chengduoZH force-pushed the feature/Add_channel_test branch from efab4f0 to 710e05d Compare January 30, 2018 09:31

follow comments

77b8872

chengduoZH force-pushed the feature/Add_channel_test branch from 710e05d to 77b8872 Compare January 30, 2018 09:39

use delete to destroy channel

ba7677e

chengduoZH force-pushed the feature/Add_channel_test branch from 4d6e884 to ba7677e Compare January 31, 2018 02:08

wangkuiyi approved these changes Jan 31, 2018

View reviewed changes

wangkuiyi merged commit adf14b0 into PaddlePaddle:develop Jan 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refine channel test #7946

Refine channel test #7946

Uh oh!

chengduoZH commented Jan 29, 2018 •

edited

Loading

typhoonzero Jan 29, 2018

chengduoZH Jan 29, 2018

typhoonzero Jan 29, 2018

chengduoZH Jan 29, 2018

chengduoZH Jan 29, 2018

typhoonzero left a comment

wangkuiyi Jan 29, 2018

wangkuiyi Jan 29, 2018

wangkuiyi left a comment

wangkuiyi Jan 30, 2018

chengduoZH Jan 30, 2018 •

edited

Loading

wangkuiyi Jan 30, 2018

wangkuiyi Jan 30, 2018

chengduoZH Jan 30, 2018

wangkuiyi Jan 30, 2018

chengduoZH Jan 30, 2018

wangkuiyi Jan 30, 2018

chengduoZH Jan 30, 2018

wangkuiyi left a comment

Labels

3 participants

	template <typename T>
	void UnBuffered<T>::Send(T* channel_element) {}

	template <typename T>
	void UnBuffered<T>::Receive(T*) {}

	template <typename T>
	UnBuffered<T>::~UnBuffered() {}

	template <typename T>
	Buffered<T>::~Buffered() {
	std::unique_lock<std::mutex> lock(mu_);
	channel_.clear();
	NotifyAllSenders(&lock);
	}

Refine channel test #7946

Refine channel test #7946

Uh oh!

Conversation

chengduoZH commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

typhoonzero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangkuiyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chengduoZH Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

wangkuiyi left a comment

Choose a reason for hiding this comment

Labels

3 participants

chengduoZH commented Jan 29, 2018 •

edited

Loading

chengduoZH Jan 30, 2018 •

edited

Loading