Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
39102:
Adding a new thread pool to SimpleThread. (Closed)
|
Created:
11 years, 9 months ago by willchan no longer on Chromium Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdded a thread pool to WorkerPool for Linux that dynamically adds threads as needed.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=12414
Patch Set 1 #Patch Set 2 : '' #
Total comments: 7
Patch Set 3 : '' #
Total comments: 41
Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 31
Patch Set 6 : '' #Patch Set 7 : '' #Patch Set 8 : '' #
Total comments: 10
Patch Set 9 : '' #Patch Set 10 : '' #
Total comments: 8
Patch Set 11 : '' #Patch Set 12 : '' #
Total comments: 1
Messages
Total messages: 20 (0 generated)
I didn't know if I should implement this worker pool directly in worker_pool_posix.cc or what not. I saw there was a thread pool already in simple_thread.h. Let me know if there's a better location for this.
I didn't know if I should implement this worker pool directly in worker_pool_posix.cc or what not. I saw there was a thread pool already in simple_thread.h. Let me know if there's a better location for this.
Hey Will, I didn't do a very good job at reviewing this, but here is my initial feedback. I didn't look closely at the implementation details yet, because there are a few things I think you need to tackle first. 1) Please double check our style guide, especially your comment style is consistently wrong. 2) I don't think this code belongs in "simple_thread". It was originally designed for testing where you didn't need a full chrome thread. Also, SimpleThread is working in Delegates (I wanted to do a Callback type thing, but there was a massive outrage for not reusing Task). Since our thread pool should be Task based, this sort of all changes. I think you should just steal the spirit of SimpleThread, and implement a ThreadPool inside of the posix stuff. We aren't going to want to reuse something like this from within simple_thread. 3) I don't see the advantage of using a hash_set, but I could be missing something. I would use a map<>, you could use a set<>, but I don't see why you would need the sorted property of a set. I think things could be made a lot simpler, give it a shot and I'll review your new changes tomorrow. Also, of course, you need tests. It's nice to do that right away in a review so a person get a feel for how the class is used, and has some examples. Thanks http://codereview.chromium.org/39102/diff/1005/9 File base/simple_thread.cc (right): http://codereview.chromium.org/39102/diff/1005/9#newcode15 Line 15: // let a SimpleThread in the DelegateSimpleThreadDynamicPool wait for work for Comment style. http://codereview.chromium.org/39102/diff/1005/9#newcode131 Line 131: DynamicSimpleThreadPool::~DynamicSimpleThreadPool() { delete impl_; } why not used a scoped_ptr http://codereview.chromium.org/39102/diff/1005/9#newcode184 Line 184: // have enough worker threads comments. http://codereview.chromium.org/39102/diff/1005/9#newcode195 Line 195: // take this opportunity to bury the other corpses at the graveyard Comments go like this. http://codereview.chromium.org/39102/diff/1005/8 File base/simple_thread.h (right): http://codereview.chromium.org/39102/diff/1005/8#newcode218 Line 218: namespace internal { Internal namespace is kinda strange. If you just want to hide it, you could define it in DynamicSimpleThreadPool? http://codereview.chromium.org/39102/diff/1005/8#newcode259 Line 259: struct hash<base::internal::WorkerThread*> { I generally am in favor of using map / set over the hash_ versions unless you can really prove there is a performance gain. Then add this compiler specific stuff required, and I feel even more strongly about it. http://codereview.chromium.org/39102/diff/1005/8#newcode313 Line 313: int num_idle_threads_; // number of threads waiting for work Comments needs to be like this. I think the end of line comments are cramping things a bit. Maybe do a single line comment above, and space out the local variables a bit.
Either I or Rietveld was being completely retarded, and I missed the unit test changes. Ignore my comments about those, sorry. On 2009/03/04 16:18:15, Dean McNamee wrote: > Hey Will, > > I didn't do a very good job at reviewing this, but here is my initial feedback. > I didn't look closely at the implementation details yet, because there are a few > things I think you need to tackle first. > > 1) Please double check our style guide, especially your comment style is > consistently wrong. > > 2) I don't think this code belongs in "simple_thread". It was originally > designed for testing where you didn't need a full chrome thread. Also, > SimpleThread is working in Delegates (I wanted to do a Callback type thing, but > there was a massive outrage for not reusing Task). Since our thread pool should > be Task based, this sort of all changes. I think you should just steal the > spirit of SimpleThread, and implement a ThreadPool inside of the posix stuff. > We aren't going to want to reuse something like this from within simple_thread. > > 3) I don't see the advantage of using a hash_set, but I could be missing > something. I would use a map<>, you could use a set<>, but I don't see why you > would need the sorted property of a set. > > I think things could be made a lot simpler, give it a shot and I'll review your > new changes tomorrow. > > Also, of course, you need tests. It's nice to do that right away in a review so > a person get a feel for how the class is used, and has some examples. > > Thanks > > http://codereview.chromium.org/39102/diff/1005/9 > File base/simple_thread.cc (right): > > http://codereview.chromium.org/39102/diff/1005/9#newcode15 > Line 15: // let a SimpleThread in the DelegateSimpleThreadDynamicPool wait for > work for > Comment style. > > http://codereview.chromium.org/39102/diff/1005/9#newcode131 > Line 131: DynamicSimpleThreadPool::~DynamicSimpleThreadPool() { delete impl_; } > why not used a scoped_ptr > > http://codereview.chromium.org/39102/diff/1005/9#newcode184 > Line 184: // have enough worker threads > comments. > > http://codereview.chromium.org/39102/diff/1005/9#newcode195 > Line 195: // take this opportunity to bury the other corpses at the graveyard > Comments go like this. > > http://codereview.chromium.org/39102/diff/1005/8 > File base/simple_thread.h (right): > > http://codereview.chromium.org/39102/diff/1005/8#newcode218 > Line 218: namespace internal { > Internal namespace is kinda strange. If you just want to hide it, you could > define it in DynamicSimpleThreadPool? > > http://codereview.chromium.org/39102/diff/1005/8#newcode259 > Line 259: struct hash<base::internal::WorkerThread*> { > I generally am in favor of using map / set over the hash_ versions unless you > can really prove there is a performance gain. Then add this compiler specific > stuff required, and I feel even more strongly about it. > > http://codereview.chromium.org/39102/diff/1005/8#newcode313 > Line 313: int num_idle_threads_; // number of threads waiting for work > Comments needs to be like this. > > I think the end of line comments are cramping things a bit. Maybe do a single > line comment above, and space out the local variables a bit.
On 2009/03/04 16:18:15, Dean McNamee wrote: > Hey Will, > > I didn't do a very good job at reviewing this, but here is my initial feedback. > I didn't look closely at the implementation details yet, because there are a few > things I think you need to tackle first. Thanks for the review! I definitely needed these high level comments, especially the one about where to actually put the code. > > 1) Please double check our style guide, especially your comment style is > consistently wrong. When you say comment style, do you primarily mean the lack of capitalization and punctuation? I definitely was lax about that, sorry. Let me know if there's anything else you're referring to in particular. Oh, and if there are other style guide violations that I'm neglecting, definitely let me know. I've probably become sloppy in my half year of not coding. > > 2) I don't think this code belongs in "simple_thread". It was originally > designed for testing where you didn't need a full chrome thread. Also, > SimpleThread is working in Delegates (I wanted to do a Callback type thing, but > there was a massive outrage for not reusing Task). Since our thread pool should > be Task based, this sort of all changes. I think you should just steal the > spirit of SimpleThread, and implement a ThreadPool inside of the posix stuff. > We aren't going to want to reuse something like this from within simple_thread. Fair enough, I wasn't sure where to put the code. > > 3) I don't see the advantage of using a hash_set, but I could be missing > something. I would use a map<>, you could use a set<>, but I don't see why you > would need the sorted property of a set. I'm confused by this statement. Are you implying that a map<> does not have the sorted property of a set? > > I think things could be made a lot simpler, give it a shot and I'll review your > new changes tomorrow. > > Also, of course, you need tests. It's nice to do that right away in a review so > a person get a feel for how the class is used, and has some examples. > > Thanks > > http://codereview.chromium.org/39102/diff/1005/9 > File base/simple_thread.cc (right): > > http://codereview.chromium.org/39102/diff/1005/9#newcode15 > Line 15: // let a SimpleThread in the DelegateSimpleThreadDynamicPool wait for > work for > Comment style. > > http://codereview.chromium.org/39102/diff/1005/9#newcode131 > Line 131: DynamicSimpleThreadPool::~DynamicSimpleThreadPool() { delete impl_; } > why not used a scoped_ptr I was thinking before that simple_thread.h might be low level enough that I should try to avoid the extra header dependency, so I just used a delete instead. Moot point now that I'll be moving it out of simple_thread.h. I'll definitely use a scoped_ptr. > > http://codereview.chromium.org/39102/diff/1005/9#newcode184 > Line 184: // have enough worker threads > comments. > > http://codereview.chromium.org/39102/diff/1005/9#newcode195 > Line 195: // take this opportunity to bury the other corpses at the graveyard > Comments go like this. > > http://codereview.chromium.org/39102/diff/1005/8 > File base/simple_thread.h (right): > > http://codereview.chromium.org/39102/diff/1005/8#newcode218 > Line 218: namespace internal { > Internal namespace is kinda strange. If you just want to hide it, you could > define it in DynamicSimpleThreadPool? Yes, that's originally how I had done so. The problem with that is accessing symbols for testability. Hence, I put it inside an internal namespace. If you look at gtest headers, they do the same thing for implementation details placed in the headers so that unittests can access the symbols. I have a question about how to test internals in stuff not defined in the interface header. Can I add another worker_pool_posix.h file where I define the implementation details? Also, how about the unittest? There's this extra thread pool implementation that ought to be unittested well, but if I put it in the main worker_pool_unittest.cc, I can't test the posix specific thread pool, unless I put the #if guards. Do we ever create separate unittest files for this? Or should I just use #if guards? They feel kludgy to me so I'd like to avoid if possible. > > http://codereview.chromium.org/39102/diff/1005/8#newcode259 > Line 259: struct hash<base::internal::WorkerThread*> { > I generally am in favor of using map / set over the hash_ versions unless you > can really prove there is a performance gain. Then add this compiler specific > stuff required, and I feel even more strongly about it. Can you explain why you are generally in favor of using map/set over the hash_ versions? Is it primarily because of this compiler specific stuff required? The rule of thumb I've heard from C++ language committee folks on internal Google mailing lists is to use the hash versions unless you need ordering, then use map/set. map/set are both larger and slower than hash_map/hash_set, so the hash versions are generally the default. I agree with the statement about compiler specific kludge. If we've decided not to go with a default hash for pointers, I wonder if we could avoid including MSVC's default hash somehow. Perhaps it's a weak symbol we could override with a definition we could set to always fail to compile. I have no clue though. > > http://codereview.chromium.org/39102/diff/1005/8#newcode313 > Line 313: int num_idle_threads_; // number of threads waiting for work > Comments needs to be like this. > > I think the end of line comments are cramping things a bit. Maybe do a single > line comment above, and space out the local variables a bit.
On 2009/03/04 17:05:50, willchan wrote: > On 2009/03/04 16:18:15, Dean McNamee wrote: > > Hey Will, > > > > I didn't do a very good job at reviewing this, but here is my initial > feedback. > > I didn't look closely at the implementation details yet, because there are a > few > > things I think you need to tackle first. > > Thanks for the review! I definitely needed these high level comments, > especially the one about where to actually put the code. > > > > > 1) Please double check our style guide, especially your comment style is > > consistently wrong. > > When you say comment style, do you primarily mean the lack of capitalization and > punctuation? I definitely was lax about that, sorry. Let me know if there's > anything else you're referring to in particular. Oh, and if there are other > style guide violations that I'm neglecting, definitely let me know. I've > probably become sloppy in my half year of not coding. > > > > > 2) I don't think this code belongs in "simple_thread". It was originally > > designed for testing where you didn't need a full chrome thread. Also, > > SimpleThread is working in Delegates (I wanted to do a Callback type thing, > but > > there was a massive outrage for not reusing Task). Since our thread pool > should > > be Task based, this sort of all changes. I think you should just steal the > > spirit of SimpleThread, and implement a ThreadPool inside of the posix stuff. > > We aren't going to want to reuse something like this from within > simple_thread. > > Fair enough, I wasn't sure where to put the code. > > > > > 3) I don't see the advantage of using a hash_set, but I could be missing > > something. I would use a map<>, you could use a set<>, but I don't see why > you > > would need the sorted property of a set. > > I'm confused by this statement. Are you implying that a map<> does not have the > sorted property of a set? Well, they both implement Unique Sorted Associative Container, so I guess not. I usually just think of sets and maps differently (although their STL implementation is similar, it's really the difference between set having the value be the key?). My point was it doesn't seem like you need any special properties, just to iterate / lookup. > > > > > I think things could be made a lot simpler, give it a shot and I'll review > your > > new changes tomorrow. > > > > Also, of course, you need tests. It's nice to do that right away in a review > so > > a person get a feel for how the class is used, and has some examples. > > > > Thanks > > > > http://codereview.chromium.org/39102/diff/1005/9 > > File base/simple_thread.cc (right): > > > > http://codereview.chromium.org/39102/diff/1005/9#newcode15 > > Line 15: // let a SimpleThread in the DelegateSimpleThreadDynamicPool wait for > > work for > > Comment style. > > > > http://codereview.chromium.org/39102/diff/1005/9#newcode131 > > Line 131: DynamicSimpleThreadPool::~DynamicSimpleThreadPool() { delete impl_; > } > > why not used a scoped_ptr > > I was thinking before that simple_thread.h might be low level enough that I > should try to avoid the extra header dependency, so I just used a delete > instead. Moot point now that I'll be moving it out of simple_thread.h. I'll > definitely use a scoped_ptr. You can still do this if you just forward declare the class. We do it all over our code. You don't need the full definition to define a scoped_ptr, since you're just defining a Blah*, which the storage size is known for even with an incomplete type. > > > > > http://codereview.chromium.org/39102/diff/1005/9#newcode184 > > Line 184: // have enough worker threads > > comments. > > > > http://codereview.chromium.org/39102/diff/1005/9#newcode195 > > Line 195: // take this opportunity to bury the other corpses at the graveyard > > Comments go like this. > > > > http://codereview.chromium.org/39102/diff/1005/8 > > File base/simple_thread.h (right): > > > > http://codereview.chromium.org/39102/diff/1005/8#newcode218 > > Line 218: namespace internal { > > Internal namespace is kinda strange. If you just want to hide it, you could > > define it in DynamicSimpleThreadPool? > > Yes, that's originally how I had done so. The problem with that is accessing > symbols for testability. Hence, I put it inside an internal namespace. If you > look at gtest headers, they do the same thing for implementation details placed > in the headers so that unittests can access the symbols. > > I have a question about how to test internals in stuff not defined in the > interface header. Can I add another worker_pool_posix.h file where I define the > implementation details? Also, how about the unittest? There's this extra > thread pool implementation that ought to be unittested well, but if I put it in > the main worker_pool_unittest.cc, I can't test the posix specific thread pool, > unless I put the #if guards. Do we ever create separate unittest files for > this? Or should I just use #if guards? They feel kludgy to me so I'd like to > avoid if possible. You don't think you'd be able to test it adequately through the public interface? > > > > > http://codereview.chromium.org/39102/diff/1005/8#newcode259 > > Line 259: struct hash<base::internal::WorkerThread*> { > > I generally am in favor of using map / set over the hash_ versions unless you > > can really prove there is a performance gain. Then add this compiler specific > > stuff required, and I feel even more strongly about it. > > Can you explain why you are generally in favor of using map/set over the hash_ > versions? Is it primarily because of this compiler specific stuff required? > The rule of thumb I've heard from C++ language committee folks on internal > Google mailing lists is to use the hash versions unless you need ordering, then > use map/set. map/set are both larger and slower than hash_map/hash_set, so the > hash versions are generally the default. > > I agree with the statement about compiler specific kludge. If we've decided not > to go with a default hash for pointers, I wonder if we could avoid including > MSVC's default hash somehow. Perhaps it's a weak symbol we could override with > a definition we could set to always fail to compile. I have no clue though. Remember we are talking about really small numbers here. What's the most number of threads we'd ever have. I'd say 20 sounds high. > > > > > http://codereview.chromium.org/39102/diff/1005/8#newcode313 > > Line 313: int num_idle_threads_; // number of threads waiting for work > > Comments needs to be like this. > > > > I think the end of line comments are cramping things a bit. Maybe do a single > > line comment above, and space out the local variables a bit.
On 2009/03/04 17:05:50, willchan wrote: > On 2009/03/04 16:18:15, Dean McNamee wrote: > > Hey Will, > > > > I didn't do a very good job at reviewing this, but here is my initial > feedback. > > I didn't look closely at the implementation details yet, because there are a > few > > things I think you need to tackle first. > > Thanks for the review! I definitely needed these high level comments, > especially the one about where to actually put the code. > > > > > 1) Please double check our style guide, especially your comment style is > > consistently wrong. > > When you say comment style, do you primarily mean the lack of capitalization and > punctuation? I definitely was lax about that, sorry. Let me know if there's > anything else you're referring to in particular. Oh, and if there are other > style guide violations that I'm neglecting, definitely let me know. I've > probably become sloppy in my half year of not coding. > > > > > 2) I don't think this code belongs in "simple_thread". It was originally > > designed for testing where you didn't need a full chrome thread. Also, > > SimpleThread is working in Delegates (I wanted to do a Callback type thing, > but > > there was a massive outrage for not reusing Task). Since our thread pool > should > > be Task based, this sort of all changes. I think you should just steal the > > spirit of SimpleThread, and implement a ThreadPool inside of the posix stuff. > > We aren't going to want to reuse something like this from within > simple_thread. > > Fair enough, I wasn't sure where to put the code. > > > > > 3) I don't see the advantage of using a hash_set, but I could be missing > > something. I would use a map<>, you could use a set<>, but I don't see why > you > > would need the sorted property of a set. > > I'm confused by this statement. Are you implying that a map<> does not have the > sorted property of a set? Well, they both implement Unique Sorted Associative Container, so I guess not. I usually just think of sets and maps differently (although their STL implementation is similar, it's really the difference between set having the value be the key?). My point was it doesn't seem like you need any special properties, just to iterate / lookup. > > > > > I think things could be made a lot simpler, give it a shot and I'll review > your > > new changes tomorrow. > > > > Also, of course, you need tests. It's nice to do that right away in a review > so > > a person get a feel for how the class is used, and has some examples. > > > > Thanks > > > > http://codereview.chromium.org/39102/diff/1005/9 > > File base/simple_thread.cc (right): > > > > http://codereview.chromium.org/39102/diff/1005/9#newcode15 > > Line 15: // let a SimpleThread in the DelegateSimpleThreadDynamicPool wait for > > work for > > Comment style. > > > > http://codereview.chromium.org/39102/diff/1005/9#newcode131 > > Line 131: DynamicSimpleThreadPool::~DynamicSimpleThreadPool() { delete impl_; > } > > why not used a scoped_ptr > > I was thinking before that simple_thread.h might be low level enough that I > should try to avoid the extra header dependency, so I just used a delete > instead. Moot point now that I'll be moving it out of simple_thread.h. I'll > definitely use a scoped_ptr. You can still do this if you just forward declare the class. We do it all over our code. You don't need the full definition to define a scoped_ptr, since you're just defining a Blah*, which the storage size is known for even with an incomplete type. > > > > > http://codereview.chromium.org/39102/diff/1005/9#newcode184 > > Line 184: // have enough worker threads > > comments. > > > > http://codereview.chromium.org/39102/diff/1005/9#newcode195 > > Line 195: // take this opportunity to bury the other corpses at the graveyard > > Comments go like this. > > > > http://codereview.chromium.org/39102/diff/1005/8 > > File base/simple_thread.h (right): > > > > http://codereview.chromium.org/39102/diff/1005/8#newcode218 > > Line 218: namespace internal { > > Internal namespace is kinda strange. If you just want to hide it, you could > > define it in DynamicSimpleThreadPool? > > Yes, that's originally how I had done so. The problem with that is accessing > symbols for testability. Hence, I put it inside an internal namespace. If you > look at gtest headers, they do the same thing for implementation details placed > in the headers so that unittests can access the symbols. > > I have a question about how to test internals in stuff not defined in the > interface header. Can I add another worker_pool_posix.h file where I define the > implementation details? Also, how about the unittest? There's this extra > thread pool implementation that ought to be unittested well, but if I put it in > the main worker_pool_unittest.cc, I can't test the posix specific thread pool, > unless I put the #if guards. Do we ever create separate unittest files for > this? Or should I just use #if guards? They feel kludgy to me so I'd like to > avoid if possible. You don't think you'd be able to test it adequately through the public interface? > > > > > http://codereview.chromium.org/39102/diff/1005/8#newcode259 > > Line 259: struct hash<base::internal::WorkerThread*> { > > I generally am in favor of using map / set over the hash_ versions unless you > > can really prove there is a performance gain. Then add this compiler specific > > stuff required, and I feel even more strongly about it. > > Can you explain why you are generally in favor of using map/set over the hash_ > versions? Is it primarily because of this compiler specific stuff required? > The rule of thumb I've heard from C++ language committee folks on internal > Google mailing lists is to use the hash versions unless you need ordering, then > use map/set. map/set are both larger and slower than hash_map/hash_set, so the > hash versions are generally the default. > > I agree with the statement about compiler specific kludge. If we've decided not > to go with a default hash for pointers, I wonder if we could avoid including > MSVC's default hash somehow. Perhaps it's a weak symbol we could override with > a definition we could set to always fail to compile. I have no clue though. Remember we are talking about really small numbers here. What's the most number of threads we'd ever have. I'd say 20 sounds high. > > > > > http://codereview.chromium.org/39102/diff/1005/8#newcode313 > > Line 313: int num_idle_threads_; // number of threads waiting for work > > Comments needs to be like this. > > > > I think the end of line comments are cramping things a bit. Maybe do a single > > line comment above, and space out the local variables a bit.
Since we switched to using non-joinable threads and moved the implementation into worker_pool_linux.cc, things changed dramatically. There are a few things I had questions on: (1) How long to keep threads idle for before exiting? (2) If I want to use a global instance of the WorkerPool, is Singleton<> the right idiom? Or should I be using pthread_once? Also, I'm not sure if there's a point to deleting this thread pool on program exit...is there? Please take another look, thanks!
This is a super good start. A bunch of small nits around the code, and a few general comments: - If DynamicThreadPool is linux specific, maybe put Linux in the class name. - I was a bit confused at first, because I kinda expected DynamicThreadPool to own the threads. I realize it's using non-joinable threads, so they're just floating, and then the threads ask the DynamicThreadPool for work. This almost seems like there is a few separate pieces, Jar wrote some code like this and he called the centralized place where people ask for work a controller. Not a big deal, just maybe add a few comments explaining the design, and that the DynamicThreadPool class is the guy that holds a lock and manages the tasks, then the non-joinable threads floating out there ask it for work. - Singleton<> is the right idiom, in terms of implementation it is probably better to use a LazyInstance<>, since in theory some other code might want to manage it's own global threadpool, and a Singleton<> would force them to share only 1 instance. The problem of using pthread_once I think is you still need to initialize the pthread structure somehow? Anyway, just use LazyInstance, should be ok. - By deleting the thread pool, do you mean deleting the DynamicThreadPool instance, or stopping the threads? I think it makes sense to delete the instance, which is what Singleton / etc will do unless you use a (bad) LeakyTraits. You will have to let the threads known that you're destroying their pool instance though, otherwise they will be left with a dangling pointer. If that's too difficult, it's probably ok to leak it. On 2009/03/10 00:24:12, willchan wrote: > Since we switched to using non-joinable threads and moved the implementation > into worker_pool_linux.cc, things changed dramatically. > > There are a few things I had questions on: > (1) How long to keep threads idle for before exiting? > (2) If I want to use a global instance of the WorkerPool, is Singleton<> the > right idiom? Or should I be using pthread_once? Also, I'm not sure if there's > a point to deleting this thread pool on program exit...is there? > > Please take another look, thanks!
Blah, Rietveld didn't send my inline comments, trying again. http://codereview.chromium.org/39102/diff/1010/2002 File base/worker_pool_linux.cc (right): http://codereview.chromium.org/39102/diff/1010/2002#newcode15 Line 15: const int kIdleSecondsBeforeExit = 60; You asked about this in the review comments. I think this can be a lot larger. I'm not sure how expensive a thread is, it takes up some memory for a stack, etc (which will be paged out). If it hasn't run for a while, I think it's probably actually cheaper to keep it not running, than to wake it up, swap it back in, and kill it. I would make this like 10 minutes or something? I guess we could try to get some data on it and figure out how often they're used. We could also always keep some alive, and only idle timeout / exit threads if we have more than we want. For example, always keep 5 threads alive, only idle timeout / exit a thread if we have more than 5. Kinda more complicated though, so probably not important right now. http://codereview.chromium.org/39102/diff/1010/2002#newcode16 Line 16: const int kWorkerThreadStackSize = 1 << 14; // 16k 16 * 1024? Then you don't even need the comment. http://codereview.chromium.org/39102/diff/1010/2002#newcode54 Line 54: base::internal::DynamicThreadPool* pool_; // not owned not owned? I see what you're saying now, but maybe you should elaborate on the comment. http://codereview.chromium.org/39102/diff/1010/2002#newcode60 Line 60: const std::string name = I think we don't normally use const on local variables, but if you think it helps your code, I'm ok with it. http://codereview.chromium.org/39102/diff/1010/2002#newcode126 Line 126: num_idle_threads_--; Can you do this counter balancing such that we don't need FirstWaitForTask? I think it's kinda confusing. http://codereview.chromium.org/39102/diff/1010/2002#newcode131 Line 131: if (tasks_.empty()) { // No work available, let's wait for work. remove the let's http://codereview.chromium.org/39102/diff/1010/2002#newcode137 Line 137: // We waited for work, but there's still no work. Slay the thread. We are not really "slaying the thread" here. We are simply returning NULL. Separate the logic. Just say, "we waited, and there is still not work, so return NULL." or whatever. http://codereview.chromium.org/39102/diff/1010/2001 File base/worker_pool_linux.h (right): http://codereview.chromium.org/39102/diff/1010/2001#newcode7 Line 7: // These symbols are exported in a header purely for testing purposes. If you want this to be a scary warning, maybe make it like NOTE: or WARNING:. Also, I think usually we start with a summary as the first part of the comment. Then put the scary note at the bottom. WARNING: This interface is only exposed for testing. You should be using WorkerPool, and not this lower level interface! http://codereview.chromium.org/39102/diff/1010/2001#newcode11 Line 11: // for a period of time to allow them to be reused. Otherwise, the threads Otherwise -> After this idle waiting period has expired http://codereview.chromium.org/39102/diff/1010/2001#newcode16 Line 16: probably only need 1 newline here. http://codereview.chromium.org/39102/diff/1010/2001#newcode34 Line 34: namespace internal { I don't think we used 'internal' for anything else. If you want your own namespace, then maybe just make your own namespace, like base::linux_thread_pool_internal, although that is a mouthful. Personally I would just put it in base, and not worry about it. If people start abusing it or it's colliding (which I doubt it would), we can deal with that later. Seems a bit paranoid and strange now. http://codereview.chromium.org/39102/diff/1010/2001#newcode38 Line 38: // All worker threads will share the same name_prefix. They will kill kill themselves sounds like kill -9, won't they just exit cleanly? http://codereview.chromium.org/39102/diff/1010/2001#newcode41 Line 41: int idle_seconds_before_exit); I was going to suggest using an Options style thing here, but I realize it's only user from one caller worker pool, so it will be easy to change the prototype. No problem. http://codereview.chromium.org/39102/diff/1010/2001#newcode44 Line 44: // ---------------------------------------------------------------- We don't usually do these ascii banners, I hate them. http://codereview.chromium.org/39102/diff/1010/2001#newcode51 Line 51: // Worker thread method to wait for up to idle_seconds_before_exit for more put pipes around variable names, |idle_seconds_before_exit| http://codereview.chromium.org/39102/diff/1010/2001#newcode55 Line 55: // Does the same thing as WaitForTask() except it also decrements Does the same thing as -> Similar to http://codereview.chromium.org/39102/diff/1010/2001#newcode56 Line 56: // num_idle_threads_. Worker threads should call this method the first time pipes http://codereview.chromium.org/39102/diff/1010/2001#newcode61 Line 61: friend class ::DynamicThreadPoolPeer; Why not have DynamicThreadPoolPeer be inside of the class http://codereview.chromium.org/39102/diff/1010/2001#newcode70 Line 70: Lock lock_; // protects all the variables below Protects ... . http://codereview.chromium.org/39102/diff/1010/2003 File base/worker_pool_linux_unittest.cc (right): http://codereview.chromium.org/39102/diff/1010/2003#newcode16 Line 16: class DynamicThreadPoolPeer { what I was saying about this being on the class, this could be namespace base { class DynamicThreadPool::DynamicThreadPoolPeer } http://codereview.chromium.org/39102/diff/1010/2003#newcode276 Line 276: EXPECT_EQ(4u, unique_threads_.size()); shouldn't 4u be 4U, or does it not matter? All of our other code uses a capital U.
http://codereview.chromium.org/39102/diff/1010/2002 File base/worker_pool_linux.cc (right): http://codereview.chromium.org/39102/diff/1010/2002#newcode15 Line 15: const int kIdleSecondsBeforeExit = 60; On 2009/03/10 11:32:14, Dean McNamee wrote: > You asked about this in the review comments. I think this can be a lot larger. > I'm not sure how expensive a thread is, it takes up some memory for a stack, etc > (which will be paged out). If it hasn't run for a while, I think it's probably > actually cheaper to keep it not running, than to wake it up, swap it back in, > and kill it. I would make this like 10 minutes or something? I guess we could > try to get some data on it and figure out how often they're used. > > We could also always keep some alive, and only idle timeout / exit threads if we > have more than we want. For example, always keep 5 threads alive, only idle > timeout / exit a thread if we have more than 5. Kinda more complicated though, > so probably not important right now. Yeah, I didn't think it was worth it to add this level of complexity. I changed this to 10 minutes. http://codereview.chromium.org/39102/diff/1010/2002#newcode16 Line 16: const int kWorkerThreadStackSize = 1 << 14; // 16k On 2009/03/10 11:32:14, Dean McNamee wrote: > 16 * 1024? Then you don't even need the comment. Done. http://codereview.chromium.org/39102/diff/1010/2002#newcode54 Line 54: base::internal::DynamicThreadPool* pool_; // not owned On 2009/03/10 11:32:14, Dean McNamee wrote: > not owned? I see what you're saying now, but maybe you should elaborate on the > comment. I didn't know how to elaborate it without making it excessively wordy. I just deleted it, since I think it's pretty natural to assume that the WorkerThread doesn't own the memory for the pool. http://codereview.chromium.org/39102/diff/1010/2002#newcode60 Line 60: const std::string name = On 2009/03/10 11:32:14, Dean McNamee wrote: > I think we don't normally use const on local variables, but if you think it > helps your code, I'm ok with it. I like using const. It doesn't matter much in this case, but if the method were longer, I think it adds to readability, since you don't have to think about whether or not the variable could have changed value somewhere in the body. This also assists the compiler in optimizing, although yes, it doesn't matter much here. http://codereview.chromium.org/39102/diff/1010/2002#newcode126 Line 126: num_idle_threads_--; On 2009/03/10 11:32:14, Dean McNamee wrote: > Can you do this counter balancing such that we don't need FirstWaitForTask? I > think it's kinda confusing. I thought about it some more, and decided to go with your recommendation and get rid of it. The crux of the problem is that when threads are first created, but before they've had a chance to call WaitForTask(), we should count them as idle threads. But if we do that, then we need a way for them to mark themselves as not idle when they first start doing work. Hence the FirstWaitForTask() method. This adds complexity, with the gain being we avoid a minor counter inaccuracy that could cause us to overprovision worker threads in a situation such as: Start with queue empty, 0 idle worker threads, 1 active worker thread. PostTask() leads to worker thread creation. At this point, we have 1 task in the queue, with |num_idle_threads_| at 0, but 1 worker thread that is beginning to enter ThreadMain(). The one active thread completes and becomes idle, bringing |num_idle_threads_| to 1. Here, we get another PostTask(). We have 2 worker threads total, only one of which counts towards |num_idle_threads|. Therefore, |tasks_.size()| is greater than |num_idle_threads|, leading to the creation of an extra unnecessary worker thread. This is a very minor edge case. It's probably very unlikely to have worker threads finish tasks and go idle _just_ as we're adding a batch of tasks. In any case, we don't underallocate threads, we just overallocate by a tad bit, which doesn't seem too bad. It also keeps the code cleaner and more maintainable. Thoughts? I've already changed it as you requested, but don't mind changing it back if you think the tradeoff isn't worth it. http://codereview.chromium.org/39102/diff/1010/2002#newcode131 Line 131: if (tasks_.empty()) { // No work available, let's wait for work. On 2009/03/10 11:32:14, Dean McNamee wrote: > remove the let's Done. http://codereview.chromium.org/39102/diff/1010/2002#newcode137 Line 137: // We waited for work, but there's still no work. Slay the thread. On 2009/03/10 11:32:14, Dean McNamee wrote: > We are not really "slaying the thread" here. We are simply returning NULL. > Separate the logic. Just say, "we waited, and there is still not work, so > return NULL." or whatever. Done. http://codereview.chromium.org/39102/diff/1010/2001 File base/worker_pool_linux.h (right): http://codereview.chromium.org/39102/diff/1010/2001#newcode7 Line 7: // These symbols are exported in a header purely for testing purposes. On 2009/03/10 11:32:14, Dean McNamee wrote: > If you want this to be a scary warning, maybe make it like NOTE: or WARNING:. > > Also, I think usually we start with a summary as the first part of the comment. > Then put the scary note at the bottom. > > WARNING: This interface is only exposed for testing. You should be using > WorkerPool, and not this lower level interface! Done. http://codereview.chromium.org/39102/diff/1010/2001#newcode11 Line 11: // for a period of time to allow them to be reused. Otherwise, the threads On 2009/03/10 11:32:14, Dean McNamee wrote: > Otherwise -> After this idle waiting period has expired Done. http://codereview.chromium.org/39102/diff/1010/2001#newcode16 Line 16: On 2009/03/10 11:32:14, Dean McNamee wrote: > probably only need 1 newline here. Done. http://codereview.chromium.org/39102/diff/1010/2001#newcode34 Line 34: namespace internal { On 2009/03/10 11:32:14, Dean McNamee wrote: > I don't think we used 'internal' for anything else. If you want your own > namespace, then maybe just make your own namespace, like > base::linux_thread_pool_internal, although that is a mouthful. > > Personally I would just put it in base, and not worry about it. If people start > abusing it or it's colliding (which I doubt it would), we can deal with that > later. Seems a bit paranoid and strange now. Done. http://codereview.chromium.org/39102/diff/1010/2001#newcode38 Line 38: // All worker threads will share the same name_prefix. They will kill On 2009/03/10 11:32:14, Dean McNamee wrote: > kill themselves sounds like kill -9, won't they just exit cleanly? I changed the comment to make it clearer. http://codereview.chromium.org/39102/diff/1010/2001#newcode44 Line 44: // ---------------------------------------------------------------- On 2009/03/10 11:32:14, Dean McNamee wrote: > We don't usually do these ascii banners, I hate them. Done. http://codereview.chromium.org/39102/diff/1010/2001#newcode51 Line 51: // Worker thread method to wait for up to idle_seconds_before_exit for more On 2009/03/10 11:32:14, Dean McNamee wrote: > put pipes around variable names, |idle_seconds_before_exit| Done. http://codereview.chromium.org/39102/diff/1010/2001#newcode55 Line 55: // Does the same thing as WaitForTask() except it also decrements On 2009/03/10 11:32:14, Dean McNamee wrote: > Does the same thing as -> Similar to Done. http://codereview.chromium.org/39102/diff/1010/2001#newcode56 Line 56: // num_idle_threads_. Worker threads should call this method the first time On 2009/03/10 11:32:14, Dean McNamee wrote: > pipes Done. http://codereview.chromium.org/39102/diff/1010/2001#newcode61 Line 61: friend class ::DynamicThreadPoolPeer; On 2009/03/10 11:32:14, Dean McNamee wrote: > Why not have DynamicThreadPoolPeer be inside of the class Done. http://codereview.chromium.org/39102/diff/1010/2001#newcode70 Line 70: Lock lock_; // protects all the variables below On 2009/03/10 11:32:14, Dean McNamee wrote: > Protects ... . Done. http://codereview.chromium.org/39102/diff/1010/2003 File base/worker_pool_linux_unittest.cc (right): http://codereview.chromium.org/39102/diff/1010/2003#newcode16 Line 16: class DynamicThreadPoolPeer { On 2009/03/10 11:32:14, Dean McNamee wrote: > what I was saying about this being on the class, this could be namespace base { > class DynamicThreadPool::DynamicThreadPoolPeer > } Done. http://codereview.chromium.org/39102/diff/1010/2003#newcode276 Line 276: EXPECT_EQ(4u, unique_threads_.size()); On 2009/03/10 11:32:14, Dean McNamee wrote: > shouldn't 4u be 4U, or does it not matter? All of our other code uses a capital > U. it doesn't matter. i grep'd and you're correct, the convention in chrome seems to be to use the capital U, although a few others use lowercase. i've changed it to U in order to be more consistent with the majority of the code.
This looks super good. The sleeps in the unittest scared me, we run unittests under valgrind / purify / on busy machines / etc, and flakey tests are really a big problem. http://codereview.chromium.org/39102/diff/3014/4004 File base/worker_pool_linux.cc (right): http://codereview.chromium.org/39102/diff/3014/4004#newcode15 Line 15: const int kIdleSecondsBeforeExit = 60 * 10; // 10 minutes 10 * 60 and you can drop the comment. http://codereview.chromium.org/39102/diff/3014/4004#newcode66 Line 66: // Task* task = pool_->FirstWaitForTask(); I like your changed to drop FirstWaitForTask, and I'm not worried about the small accounting race where we don't count the thread as idle. So I'm all for dropping the FirstWaitForTask. http://codereview.chromium.org/39102/diff/3014/4004#newcode71 Line 71: } Now that you just have WaitForTask, how about rewriting the loop as something like for (;;) { Task* task = pool_->WaitForTask() if (!task) break; task->Run(); delete task; } or even while (Task* task = pool_->WaitForTask()), but I'm not sure we usually do things like that. http://codereview.chromium.org/39102/diff/3014/4004#newcode73 Line 73: delete this; maybe comment this. http://codereview.chromium.org/39102/diff/3014/4004#newcode79 Line 79: remove this empty line http://codereview.chromium.org/39102/diff/3014/4004#newcode81 Line 81: static void New(void* instance) { You could also inherit from DefaultLazyInstanceTrait, and just override Delete() ? I think that would be better, so that you aren't duplicating the New() code. Or you could also just call DefaultLazyInstanceTraits<WorkerPoolImpl>::New(); http://codereview.chromium.org/39102/diff/3014/4004#newcode87 Line 87: base::LazyInstance<WorkerPoolImpl, LeakyWorkerPoolImplTraits> lazy_worker_pool( This is in a namespace, so if you want to be less wordy you can just say LeakyTraits or something http://codereview.chromium.org/39102/diff/3014/4004#newcode94 Line 94: lazy_worker_pool.Pointer()->PostTask(from_here, task, task_is_slow); this should be like g_worker_pool or something http://codereview.chromium.org/39102/diff/3014/4004#newcode105 Line 105: tasks_available_cv_(&lock_), My C++ is probably not so great, will lock_ be completely constructed when you're creating the cv? http://codereview.chromium.org/39102/diff/3014/4004#newcode118 Line 118: if (static_cast<size_t>(num_idle_threads_) >= tasks_.size()) { hmm, not sure what I think about it, but what about making num_idle_threads_ a size_t ? :( http://codereview.chromium.org/39102/diff/3014/4004#newcode123 Line 123: PlatformThread::CreateNonJoinable(kWorkerThreadStackSize, worker); Maybe comment that the thread will take ownership of the WorkerThread and delete it on thread exit? http://codereview.chromium.org/39102/diff/3014/4003 File base/worker_pool_linux.h (right): http://codereview.chromium.org/39102/diff/3014/4003#newcode14 Line 14: // LinuxDynamicThreadPool for work and eventually clean themselves up. This comment is great, thanks. http://codereview.chromium.org/39102/diff/3014/4003#newcode40 Line 40: // All worker threads will share the same name_prefix. They will exit after |name_prefix| http://codereview.chromium.org/39102/diff/3014/4003#newcode46 Line 46: // Adds a |task| to the thread pool. LinuxDynamicThreadPool assumes ownership Adds |task|, not a task? http://codereview.chromium.org/39102/diff/3014/4005 File base/worker_pool_linux_unittest.cc (right): http://codereview.chromium.org/39102/diff/3014/4005#newcode67 Line 67: AutoLock l(*unique_threads_lock_); locked http://codereview.chromium.org/39102/diff/3014/4005#newcode100 Line 100: AutoLock l(*counter_lock_); locked http://codereview.chromium.org/39102/diff/3014/4005#newcode117 Line 117: : pool_("dynamic_pool", 60*60), 60 * 60 http://codereview.chromium.org/39102/diff/3014/4005#newcode130 Line 130: PlatformThread::Sleep(100); Is there anyway to synchronize this instead of sleeping? We really want to avoid flakey tests. http://codereview.chromium.org/39102/diff/3014/4005#newcode169 Line 169: PlatformThread::Sleep(10); Sleeping ... :(
Thanks for the careful review. I totally agree with your comment about the sleeps. In order to work around that, I added a condition variable into the thread pool which is only used in tests. The cost should be minimal, so hopefully it seems reasonable to you. It makes the tests a lot cleaner. I also ran into trouble with the thread pool cleanup in the tests, so I just spent the time and made the thread pool cleanup properly (used scoped_refptr). http://codereview.chromium.org/39102/diff/3014/4004 File base/worker_pool_linux.cc (right): http://codereview.chromium.org/39102/diff/3014/4004#newcode15 Line 15: const int kIdleSecondsBeforeExit = 60 * 10; // 10 minutes On 2009/03/11 11:17:26, Dean McNamee wrote: > 10 * 60 and you can drop the comment. Done. http://codereview.chromium.org/39102/diff/3014/4004#newcode71 Line 71: } On 2009/03/11 11:17:26, Dean McNamee wrote: > Now that you just have WaitForTask, how about rewriting the loop as something > like > > for (;;) { > Task* task = pool_->WaitForTask() > if (!task) > break; > task->Run(); > delete task; > } > > or even while (Task* task = pool_->WaitForTask()), but I'm not sure we usually > do things like that. Done. http://codereview.chromium.org/39102/diff/3014/4004#newcode73 Line 73: delete this; On 2009/03/11 11:17:26, Dean McNamee wrote: > maybe comment this. Done. http://codereview.chromium.org/39102/diff/3014/4004#newcode79 Line 79: On 2009/03/11 11:17:26, Dean McNamee wrote: > remove this empty line Done. http://codereview.chromium.org/39102/diff/3014/4004#newcode81 Line 81: static void New(void* instance) { On 2009/03/11 11:17:26, Dean McNamee wrote: > You could also inherit from DefaultLazyInstanceTrait, and just override Delete() > ? I think that would be better, so that you aren't duplicating the New() code. > Or you could also just call DefaultLazyInstanceTraits<WorkerPoolImpl>::New(); I'm not leaking anymore so this is irrelevant. Inheritance doesn't work though, since you can't override static methods. http://codereview.chromium.org/39102/diff/3014/4004#newcode87 Line 87: base::LazyInstance<WorkerPoolImpl, LeakyWorkerPoolImplTraits> lazy_worker_pool( On 2009/03/11 11:17:26, Dean McNamee wrote: > This is in a namespace, so if you want to be less wordy you can just say > LeakyTraits or something Irrelevant since it's not leaky anymore. http://codereview.chromium.org/39102/diff/3014/4004#newcode94 Line 94: lazy_worker_pool.Pointer()->PostTask(from_here, task, task_is_slow); On 2009/03/11 11:17:26, Dean McNamee wrote: > this should be like g_worker_pool or something ah yes, i didn't know if you guys used that convention or not. glad to hear you do. done. http://codereview.chromium.org/39102/diff/3014/4004#newcode105 Line 105: tasks_available_cv_(&lock_), On 2009/03/11 11:17:26, Dean McNamee wrote: > My C++ is probably not so great, will lock_ be completely constructed when > you're creating the cv? yep. the construction order is whatever is declared first in the class definition is constructed first. destruction order is the reverse. http://codereview.chromium.org/39102/diff/3014/4004#newcode118 Line 118: if (static_cast<size_t>(num_idle_threads_) >= tasks_.size()) { On 2009/03/11 11:17:26, Dean McNamee wrote: > hmm, not sure what I think about it, but what about making num_idle_threads_ a > size_t ? :( i don't know much about when or when not to use a size_t, but my impression was it's intended for sizes. i honestly don't know though. if you prefer, i'm happy to change it to a size_t. http://codereview.chromium.org/39102/diff/3014/4004#newcode123 Line 123: PlatformThread::CreateNonJoinable(kWorkerThreadStackSize, worker); On 2009/03/11 11:17:26, Dean McNamee wrote: > Maybe comment that the thread will take ownership of the WorkerThread and delete > it on thread exit? Done. http://codereview.chromium.org/39102/diff/3014/4003 File base/worker_pool_linux.h (right): http://codereview.chromium.org/39102/diff/3014/4003#newcode40 Line 40: // All worker threads will share the same name_prefix. They will exit after On 2009/03/11 11:17:26, Dean McNamee wrote: > |name_prefix| Done. http://codereview.chromium.org/39102/diff/3014/4003#newcode46 Line 46: // Adds a |task| to the thread pool. LinuxDynamicThreadPool assumes ownership On 2009/03/11 11:17:26, Dean McNamee wrote: > Adds |task|, not a task? Done.
This looks great to me, I would swing it by Darin and make sure I didn't miss any details. Thanks http://codereview.chromium.org/39102/diff/4020/4022 File base/worker_pool_linux.cc (right): http://codereview.chromium.org/39102/diff/4020/4022#newcode4 Line 4: // blank line not // line http://codereview.chromium.org/39102/diff/4020/4022#newcode113 Line 113: DCHECK(!terminated_) << "Thread pool is already terminated."; You can drop the extra string here, it bloats our binaries, and the line number / etc will be enough. http://codereview.chromium.org/39102/diff/4020/4022#newcode122 Line 122: << "This thread pool is already terminated. Do not post new tasks."; Same, I think we try to avoid extra << on DCHECKs. http://codereview.chromium.org/39102/diff/4020/4022#newcode141 Line 141: if (terminated_) return NULL; We never do this one 1 line I think. It should be two. http://codereview.chromium.org/39102/diff/4020/4021 File base/worker_pool_linux.h (right): http://codereview.chromium.org/39102/diff/4020/4021#newcode41 Line 41: class LinuxDynamicThreadPool : public RefCounted<LinuxDynamicThreadPool> { This needs to be RefCountedThreadSafe, no?
[ +darin ] http://codereview.chromium.org/39102/diff/4020/4022 File base/worker_pool_linux.cc (right): http://codereview.chromium.org/39102/diff/4020/4022#newcode4 Line 4: // On 2009/03/12 11:04:17, Dean McNamee wrote: > blank line not // line Done. http://codereview.chromium.org/39102/diff/4020/4022#newcode113 Line 113: DCHECK(!terminated_) << "Thread pool is already terminated."; On 2009/03/12 11:04:17, Dean McNamee wrote: > You can drop the extra string here, it bloats our binaries, and the line number > / etc will be enough. Which binaries does it bloat? Just the debug ones, right? Shouldn't the compiler optimize it out in an optimized build since the DCHECK itself is optimized out? http://codereview.chromium.org/39102/diff/4020/4022#newcode122 Line 122: << "This thread pool is already terminated. Do not post new tasks."; On 2009/03/12 11:04:17, Dean McNamee wrote: > Same, I think we try to avoid extra << on DCHECKs. willchan@panda:/usr/local/google/chromium/src/base$ chromesearch "DCHECK\(.*\) <<" | wc -l 252 Is this legacy code before we started trying to avoid extra <<'s? http://codereview.chromium.org/39102/diff/4020/4022#newcode141 Line 141: if (terminated_) return NULL; On 2009/03/12 11:04:17, Dean McNamee wrote: > We never do this one 1 line I think. It should be two. Looks like it's done, but not often, especially given how often if statements are used. Changed. willchan@panda:/usr/local/google/chromium/src/base$ chromesearch -f "(trunk/src/base|trunk/src/chrome).*\.(cc|h)" "if \(.*\) \w" | wc -l 84 http://codereview.chromium.org/39102/diff/4020/4021 File base/worker_pool_linux.h (right): http://codereview.chromium.org/39102/diff/4020/4021#newcode41 Line 41: class LinuxDynamicThreadPool : public RefCounted<LinuxDynamicThreadPool> { On 2009/03/12 11:04:17, Dean McNamee wrote: > This needs to be RefCountedThreadSafe, no? Thanks for the catch! Sorry, I didn't know there was a threadsafe variant. I should have read the code more carefully. As an aside, is there a reason we don't have shared_ptr?
I updated this changelist because I found out that it caused net_unittests to break since the 16k thread stack size was too small. Apparently the implementation of getaddrinfo uses a large stack. I bumped it up to 64k. Darin, mind taking a look too? Thanks.
LGTM! just some minor style nits: http://codereview.chromium.org/39102/diff/5001/4030 File base/worker_pool_linux.cc (right): http://codereview.chromium.org/39102/diff/5001/4030#newcode74 Line 74: if (!task) break; style-nit: please put break on a new line http://codereview.chromium.org/39102/diff/5001/4030#newcode147 Line 147: if (num_idle_threads_cv_.get()) num_idle_threads_cv_->Signal(); nit: insert a new line for the body of the if http://codereview.chromium.org/39102/diff/5001/4030#newcode151 Line 151: if (num_idle_threads_cv_.get()) num_idle_threads_cv_->Signal(); ditto http://codereview.chromium.org/39102/diff/5001/4031 File base/worker_pool_linux_unittest.cc (right): http://codereview.chromium.org/39102/diff/5001/4031#newcode191 Line 191: << "There should be only one thread allocated for one task."; nit: it is conventional in chrome code to put the operator on the preceding line when wrapping an expression.
Thanks for the comments. I've addressed them all and searched through the code to make sure I'm compliant with these conventions (fixed one other instance of the operator on the preceding line convention). http://codereview.chromium.org/39102/diff/5001/4030 File base/worker_pool_linux.cc (right): http://codereview.chromium.org/39102/diff/5001/4030#newcode74 Line 74: if (!task) break; On 2009/03/24 17:50:44, darin wrote: > style-nit: please put break on a new line Done. http://codereview.chromium.org/39102/diff/5001/4030#newcode147 Line 147: if (num_idle_threads_cv_.get()) num_idle_threads_cv_->Signal(); On 2009/03/24 17:50:44, darin wrote: > nit: insert a new line for the body of the if Done. http://codereview.chromium.org/39102/diff/5001/4030#newcode151 Line 151: if (num_idle_threads_cv_.get()) num_idle_threads_cv_->Signal(); On 2009/03/24 17:50:44, darin wrote: > ditto Done. http://codereview.chromium.org/39102/diff/5001/4031 File base/worker_pool_linux_unittest.cc (right): http://codereview.chromium.org/39102/diff/5001/4031#newcode191 Line 191: << "There should be only one thread allocated for one task."; On 2009/03/24 17:50:44, darin wrote: > nit: it is conventional in chrome code to put the operator on the preceding line > when wrapping an expression. Done.
LGTM
http://codereview.chromium.org/39102/diff/6003/base/base.gyp File base/base.gyp (right): http://codereview.chromium.org/39102/diff/6003/base/base.gyp#newcode571 base/base.gyp:571: 'worker_pool_linux_unittest.cc', did you intentionally add this to the "sources!" section? (i.e. the list of sources to exclude on linux) I have a CL out that adds this to the real sources list – miraculously the file still compiles and all tests still pass. Please let me know if this was in fact intentional. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
