Issue 6777064: Add SkThreadPool for managing threads. (Closed)
DescriptionAdd SkThreadPool for managing threads.
Skia-ized from https://codereview.appspot.com/6755043/
TODO: Use SkThread and platform independent features.
Committed: https://code.google.com/p/skia/source/detail?r=6217
Patch Set 1 #
Total comments: 36
Patch Set 2 : Respond to comments #
Total comments: 2
Patch Set 3 : More fixes. Also use an SkTDLinkedList for the queue. #
Total comments: 8
Patch Set 4 : Lock for broadcast and keep SkRunnable interface simple. #Patch Set 5 : #
Total comments: 1
MessagesTotal messages: 10
Here's a patch that moves your code into skia. It's mostly just renaming with some usage of Skia classes. I stuck with using an SkTDArray instead of an SkTDLinkedList for the threads, since the linked list requires modifying the class of the object that is stored in the list. Feel free to use or ignore if you've already started on moving your code into Skia.
Sign in to reply to this message.
Generally looks great. I have a zillion little questions. I'm not sure I'm one to pick on you about the Skia style guide, but I pointed out a few places I think you may have glossed over. I inlined the responses to your questions on that other issue so that we can consolidate the conversations all in one place. Hope this isn't too confusing. https://codereview.appspot.com/6777064/diff/1/include/utils/SkCondVar.h File include/utils/SkCondVar.h (right): https://codereview.appspot.com/6777064/diff/1/include/utils/SkCondVar.h#newco... include/utils/SkCondVar.h:28: pthread_mutex_t mutex_; While I personally quite dig this naming scheme, don't we want fMutex and fCond, lock(), unlock(), wait(), and signal()? https://codereview.appspot.com/6777064/diff/1/include/utils/SkCountdown.h File include/utils/SkCountdown.h (right): https://codereview.appspot.com/6777064/diff/1/include/utils/SkCountdown.h#new... include/utils/SkCountdown.h:16: explicit SkCountdown(int n); n -> count for consistency with fCount (and Countdown)? https://codereview.appspot.com/6777064/diff/1/include/utils/SkCountdown.h#new... include/utils/SkCountdown.h:18: virtual void Run(); SK_OVERRIDE goes in here somewhere right? https://codereview.appspot.com/6777064/diff/1/include/utils/SkCountdown.h#new... include/utils/SkCountdown.h:21: void Wait(); -> run(), wait() ? https://codereview.appspot.com/6777064/diff/1/include/utils/SkRunnable.h File include/utils/SkRunnable.h (right): https://codereview.appspot.com/6777064/diff/1/include/utils/SkRunnable.h#newc... include/utils/SkRunnable.h:13: virtual void Run() = 0; -> run()? https://codereview.appspot.com/6777064/diff/1/include/utils/SkThreadPool.h File include/utils/SkThreadPool.h (right): https://codereview.appspot.com/6777064/diff/1/include/utils/SkThreadPool.h#ne... include/utils/SkThreadPool.h:20: // Create a threadpool with exactly num_threads (>=0) threads. Funny indent? https://codereview.appspot.com/6777064/diff/1/include/utils/SkThreadPool.h#ne... include/utils/SkThreadPool.h:26: void Add(SkRunnable*); Yes, we could definitely pass in a SkRunnable to run on each thread, either in the constructor or from another method. It's not clear to me though that this is necessary, at least in this case; isn't the standard thread-local idiom to just do this lazily the first time around, something like this in the main SkRunnable: if (0 == GetTLSFontCacheLimit()) { SetTLSFontCacheLimit(BYTES); } SkGlyphCache_Globals& cache = SkGlyphCache_Globals::GetTLS(); ... or maybe just SkGlyphCache_Globals& cache = SkGlyphCache_Globals::GetTLS(); cache.setFontCacheLimit(BYTES); ... as setFontCacheLimit appears to be idempotent if BYTES doesn't increase. https://codereview.appspot.com/6777064/diff/1/include/utils/SkThreadPool.h#ne... include/utils/SkThreadPool.h:26: void Add(SkRunnable*); Same deal, -> add(), addInternal() ? https://codereview.appspot.com/6777064/diff/1/src/utils/SkCountdown.cpp File src/utils/SkCountdown.cpp (right): https://codereview.appspot.com/6777064/diff/1/src/utils/SkCountdown.cpp#newco... src/utils/SkCountdown.cpp:14: fCount--; You're right, we do only need to signal when fCount hits 0. This is a brain fart on my part. Either atomic decrement or -- when holding the CondVar's lock should work fine for decrementing fCount. As long as we're not oversignaling as I first wrote it, I bet we won't notice a performance difference between the two. https://codereview.appspot.com/6777064/diff/1/src/utils/SkCountdown.cpp#newco... src/utils/SkCountdown.cpp:22: fReady.Wait(); Yes, Wait does lock once it resumes. We could call fReady.Unlock() here for consistency, though it's a little moot: we've not provided a way to reset the SkCountdown, so there's no way you can sensibly call Wait() twice anyway. Want to add an .Unlock() call here and a Reset(int) method? https://codereview.appspot.com/6777064/diff/1/src/utils/SkThreadPool.cpp File src/utils/SkThreadPool.cpp (right): https://codereview.appspot.com/6777064/diff/1/src/utils/SkThreadPool.cpp#newc... src/utils/SkThreadPool.cpp:25: AddInternal(NULL); Am I reading it right that the style guide wants this to be this->addInternal(NULL) ? https://codereview.appspot.com/6777064/diff/1/src/utils/SkThreadPool.cpp#newc... src/utils/SkThreadPool.cpp:43: // Wait yields the lock while waiting, but will have it again when awoken. Indentation got a little wacky starting here until just before return NULL; https://codereview.appspot.com/6777064/diff/1/src/utils/SkThreadPool.cpp#newc... src/utils/SkThreadPool.cpp:51: pool->fQueue.pop(&r); Ah, so we're running the SkRunnables in LIFO order instead of FIFO. That doesn't particularly matter, but we should probably clean up all the references to the word queue so no one gets confused reading it thinking they'll run in queue order. https://codereview.appspot.com/6777064/diff/1/src/utils/SkThreadPool.cpp#newc... src/utils/SkThreadPool.cpp:68: return NULL; // Unreachable. The only exit happens when r == NULL. -> NULL == r? https://codereview.appspot.com/6777064/diff/1/src/utils/SkThreadPool.cpp#newc... src/utils/SkThreadPool.cpp:72: // We don't want clients killing off our threads by passing NULL, now do we? extra space before NULL?
Sign in to reply to this message.
I've uploaded a new patchset with some changes. I still need to switch to an actual queue (or change the names), in addition to implementing a windows version. I'm also wondering if SkCondVar should be combined with SkMutex somehow. https://codereview.appspot.com/6777064/diff/1/include/utils/SkCondVar.h File include/utils/SkCondVar.h (right): https://codereview.appspot.com/6777064/diff/1/include/utils/SkCondVar.h#newco... include/utils/SkCondVar.h:28: pthread_mutex_t mutex_; On 2012/10/28 20:30:32, mtklein wrote: > While I personally quite dig this naming scheme, don't we want fMutex and fCond, > lock(), unlock(), wait(), and signal()? Absolutely. https://codereview.appspot.com/6777064/diff/1/include/utils/SkCountdown.h File include/utils/SkCountdown.h (right): https://codereview.appspot.com/6777064/diff/1/include/utils/SkCountdown.h#new... include/utils/SkCountdown.h:16: explicit SkCountdown(int n); On 2012/10/28 20:30:32, mtklein wrote: > n -> count for consistency with fCount (and Countdown)? Done. https://codereview.appspot.com/6777064/diff/1/include/utils/SkCountdown.h#new... include/utils/SkCountdown.h:18: virtual void Run(); On 2012/10/28 20:30:32, mtklein wrote: > SK_OVERRIDE goes in here somewhere right? Done. https://codereview.appspot.com/6777064/diff/1/include/utils/SkCountdown.h#new... include/utils/SkCountdown.h:21: void Wait(); On 2012/10/28 20:30:32, mtklein wrote: > -> run(), wait() ? Done. https://codereview.appspot.com/6777064/diff/1/include/utils/SkRunnable.h File include/utils/SkRunnable.h (right): https://codereview.appspot.com/6777064/diff/1/include/utils/SkRunnable.h#newc... include/utils/SkRunnable.h:13: virtual void Run() = 0; On 2012/10/28 20:30:32, mtklein wrote: > -> run()? Done. https://codereview.appspot.com/6777064/diff/1/include/utils/SkThreadPool.h File include/utils/SkThreadPool.h (right): https://codereview.appspot.com/6777064/diff/1/include/utils/SkThreadPool.h#ne... include/utils/SkThreadPool.h:20: // Create a threadpool with exactly num_threads (>=0) threads. On 2012/10/28 20:30:32, mtklein wrote: > Funny indent? Done. https://codereview.appspot.com/6777064/diff/1/include/utils/SkThreadPool.h#ne... include/utils/SkThreadPool.h:26: void Add(SkRunnable*); On 2012/10/28 20:30:32, mtklein wrote: > Yes, we could definitely pass in a SkRunnable to run on each thread, either in > the constructor or from another method. It's not clear to me though that this > is necessary, at least in this case; isn't the standard thread-local idiom to > just do this lazily the first time around, something like this in the main > SkRunnable: > > if (0 == GetTLSFontCacheLimit()) { > SetTLSFontCacheLimit(BYTES); > } > SkGlyphCache_Globals& cache = SkGlyphCache_Globals::GetTLS(); > ... > > or maybe just > > SkGlyphCache_Globals& cache = SkGlyphCache_Globals::GetTLS(); > cache.setFontCacheLimit(BYTES); > ... > > as setFontCacheLimit appears to be idempotent if BYTES doesn't increase. That's correct. We could even just call GetTLS(), since an SkGlyphCache gets initialized with a limit anyway (unless we want a particular amount). I was trying to avoid making the same call repeatedly when it only needs to be called the first time, but it may be cheap enough that we don't need to worry about it. https://codereview.appspot.com/6777064/diff/1/include/utils/SkThreadPool.h#ne... include/utils/SkThreadPool.h:26: void Add(SkRunnable*); On 2012/10/28 20:30:32, mtklein wrote: > Same deal, -> add(), addInternal() ? Done. https://codereview.appspot.com/6777064/diff/1/src/utils/SkCountdown.cpp File src/utils/SkCountdown.cpp (right): https://codereview.appspot.com/6777064/diff/1/src/utils/SkCountdown.cpp#newco... src/utils/SkCountdown.cpp:14: fCount--; On 2012/10/28 20:30:32, mtklein wrote: > You're right, we do only need to signal when fCount hits 0. This is a brain > fart on my part. Either atomic decrement or -- when holding the CondVar's lock > should work fine for decrementing fCount. As long as we're not oversignaling as > I first wrote it, I bet we won't notice a performance difference between the > two. The advantage to using an atomic decrement is that we can use intrinsics when available, which I believe should be faster. https://codereview.appspot.com/6777064/diff/1/src/utils/SkCountdown.cpp#newco... src/utils/SkCountdown.cpp:22: fReady.Wait(); On 2012/10/28 20:30:32, mtklein wrote: > Yes, Wait does lock once it resumes. We could call fReady.Unlock() here for > consistency, though it's a little moot: we've not provided a way to reset the > SkCountdown, so there's no way you can sensibly call Wait() twice anyway. Want > to add an .Unlock() call here and a Reset(int) method? I actually did in my change where I'm using it, but left it out of this one. https://codereview.appspot.com/6777064/diff/1/src/utils/SkThreadPool.cpp File src/utils/SkThreadPool.cpp (right): https://codereview.appspot.com/6777064/diff/1/src/utils/SkThreadPool.cpp#newc... src/utils/SkThreadPool.cpp:25: AddInternal(NULL); On 2012/10/28 20:30:32, mtklein wrote: > Am I reading it right that the style guide wants this to be > this->addInternal(NULL) ? Yes. https://codereview.appspot.com/6777064/diff/1/src/utils/SkThreadPool.cpp#newc... src/utils/SkThreadPool.cpp:43: // Wait yields the lock while waiting, but will have it again when awoken. On 2012/10/28 20:30:32, mtklein wrote: > Indentation got a little wacky starting here until just before return NULL; Done. https://codereview.appspot.com/6777064/diff/1/src/utils/SkThreadPool.cpp#newc... src/utils/SkThreadPool.cpp:51: pool->fQueue.pop(&r); On 2012/10/28 20:30:32, mtklein wrote: > Ah, so we're running the SkRunnables in LIFO order instead of FIFO. That > doesn't particularly matter, but we should probably clean up all the references > to the word queue so no one gets confused reading it thinking they'll run in > queue order. D'oh! I agree that it probably doesn't matter, but it does seem more appropriate that they should run in FIFO. https://codereview.appspot.com/6777064/diff/1/src/utils/SkThreadPool.cpp#newc... src/utils/SkThreadPool.cpp:68: return NULL; // Unreachable. The only exit happens when r == NULL. On 2012/10/28 20:30:32, mtklein wrote: > -> NULL == r? Probably doesn't matter, since this is just a comment (I assume the motivation is to avoid an accidental assignment when you intended to do a comparison), but it doesn't hurt to be consistent. https://codereview.appspot.com/6777064/diff/1/src/utils/SkThreadPool.cpp#newc... src/utils/SkThreadPool.cpp:72: // We don't want clients killing off our threads by passing NULL, now do we? On 2012/10/28 20:30:32, mtklein wrote: > extra space before NULL? Done.
Sign in to reply to this message.
LGTM Cool, this LGTM in the sense that it appears it won't blow anything up if checked in. Up to you to switch to the queue now or in a follow up. The annoying thing about combining SkCondVar and SkMutex is that we really need access to both the pthread_cond_t and pthread_mutex_t when we call wait(). Any abstraction I can think of between the two is going to be pretty leaky. I'm not sure there's any real value, say, in having SkCondVar inherit from SkMutex and making the pthread_mutex_t protected. It may end up that this gets clearer once we've figured out how to implement this stuff on Windows. I get the sense reading through some docs that we can piggyback on a higher-level threadpool implementation there, and might not have to mess with (or even have access to) condition variables. What do you say to starting with a Windows implementation where add() calls run() immediately on the calling thread as we do for 0 threads, or one that spawns and then joins the threads all inside add()? We just need something simple that runs correctly on Windows, and then we can make it better. https://codereview.appspot.com/6777064/diff/1/include/utils/SkThreadPool.h File include/utils/SkThreadPool.h (right): https://codereview.appspot.com/6777064/diff/1/include/utils/SkThreadPool.h#ne... include/utils/SkThreadPool.h:26: void Add(SkRunnable*); On 2012/10/29 18:27:31, scroggo-work wrote: > On 2012/10/28 20:30:32, mtklein wrote: > > Yes, we could definitely pass in a SkRunnable to run on each thread, either in > > the constructor or from another method. It's not clear to me though that this > > is necessary, at least in this case; isn't the standard thread-local idiom to > > just do this lazily the first time around, something like this in the main > > SkRunnable: > > > > if (0 == GetTLSFontCacheLimit()) { > > SetTLSFontCacheLimit(BYTES); > > } > > SkGlyphCache_Globals& cache = SkGlyphCache_Globals::GetTLS(); > > ... > > > > or maybe just > > > > SkGlyphCache_Globals& cache = SkGlyphCache_Globals::GetTLS(); > > cache.setFontCacheLimit(BYTES); > > ... > > > > as setFontCacheLimit appears to be idempotent if BYTES doesn't increase. > > That's correct. We could even just call GetTLS(), since an SkGlyphCache gets > initialized with a limit anyway (unless we want a particular amount). I was > trying to avoid making the same call repeatedly when it only needs to be called > the first time, but it may be cheap enough that we don't need to worry about it. Yeah, I think this might be something we should worry about only when it presents itself as a problem. https://codereview.appspot.com/6777064/diff/1/src/utils/SkCountdown.cpp File src/utils/SkCountdown.cpp (right): https://codereview.appspot.com/6777064/diff/1/src/utils/SkCountdown.cpp#newco... src/utils/SkCountdown.cpp:14: fCount--; On 2012/10/29 18:27:31, scroggo-work wrote: > On 2012/10/28 20:30:32, mtklein wrote: > > You're right, we do only need to signal when fCount hits 0. This is a brain > > fart on my part. Either atomic decrement or -- when holding the CondVar's > lock > > should work fine for decrementing fCount. As long as we're not oversignaling > as > > I first wrote it, I bet we won't notice a performance difference between the > > two. > > The advantage to using an atomic decrement is that we can use intrinsics when > available, which I believe should be faster. SGTM https://codereview.appspot.com/6777064/diff/1/src/utils/SkCountdown.cpp#newco... src/utils/SkCountdown.cpp:22: fReady.Wait(); On 2012/10/29 18:27:31, scroggo-work wrote: > On 2012/10/28 20:30:32, mtklein wrote: > > Yes, Wait does lock once it resumes. We could call fReady.Unlock() here for > > consistency, though it's a little moot: we've not provided a way to reset the > > SkCountdown, so there's no way you can sensibly call Wait() twice anyway. > Want > > to add an .Unlock() call here and a Reset(int) method? > > I actually did in my change where I'm using it, but left it out of this one. Cool. Just a suggestion: even though reset() is tiny, it might help readability to have it defined here in the .cpp also. https://codereview.appspot.com/6777064/diff/1/src/utils/SkThreadPool.cpp File src/utils/SkThreadPool.cpp (right): https://codereview.appspot.com/6777064/diff/1/src/utils/SkThreadPool.cpp#newc... src/utils/SkThreadPool.cpp:51: pool->fQueue.pop(&r); On 2012/10/29 18:27:31, scroggo-work wrote: > On 2012/10/28 20:30:32, mtklein wrote: > > Ah, so we're running the SkRunnables in LIFO order instead of FIFO. That > > doesn't particularly matter, but we should probably clean up all the > references > > to the word queue so no one gets confused reading it thinking they'll run in > > queue order. > > D'oh! I agree that it probably doesn't matter, but it does seem more appropriate > that they should run in FIFO. You've got an SkDeque that looks usable, though it somewhat awkwardly deals in void* instead of being templated. There's always the option of just creating a adapter struct to use as the node for SkTDLinkedList. https://codereview.appspot.com/6777064/diff/1/src/utils/SkThreadPool.cpp#newc... src/utils/SkThreadPool.cpp:68: return NULL; // Unreachable. The only exit happens when r == NULL. On 2012/10/29 18:27:31, scroggo-work wrote: > On 2012/10/28 20:30:32, mtklein wrote: > > -> NULL == r? > > Probably doesn't matter, since this is just a comment (I assume the motivation > is to avoid an accidental assignment when you intended to do a comparison), but > it doesn't hurt to be consistent. :) I was just trying to make it easier for the reader to find the snippet above. https://codereview.appspot.com/6777064/diff/8001/include/utils/SkThreadPool.h File include/utils/SkThreadPool.h (right): https://codereview.appspot.com/6777064/diff/8001/include/utils/SkThreadPool.h... include/utils/SkThreadPool.h:34: * Same as Add, but doesn't swallow NULL. -> add :)
Sign in to reply to this message.
New patch, with SkTDLinkedList for the queue. It doesn't allow me to add a NULL runnable, so I've changed the method of shutting down the threads. Let me know what you think. I'd like to look into the Windows implementation before putting in a dummy version. Skia's tree is somewhat closed at the moment anyway, due to Chrome branching. https://codereview.appspot.com/6777064/diff/1/src/utils/SkCountdown.cpp File src/utils/SkCountdown.cpp (right): https://codereview.appspot.com/6777064/diff/1/src/utils/SkCountdown.cpp#newco... src/utils/SkCountdown.cpp:22: fReady.Wait(); On 2012/10/29 19:11:09, mtklein wrote: > On 2012/10/29 18:27:31, scroggo-work wrote: > > On 2012/10/28 20:30:32, mtklein wrote: > > > Yes, Wait does lock once it resumes. We could call fReady.Unlock() here for > > > consistency, though it's a little moot: we've not provided a way to reset > the > > > SkCountdown, so there's no way you can sensibly call Wait() twice anyway. > > Want > > > to add an .Unlock() call here and a Reset(int) method? > > > > I actually did in my change where I'm using it, but left it out of this one. > > Cool. Just a suggestion: even though reset() is tiny, it might help readability > to have it defined here in the .cpp also. Done. https://codereview.appspot.com/6777064/diff/8001/include/utils/SkThreadPool.h File include/utils/SkThreadPool.h (right): https://codereview.appspot.com/6777064/diff/8001/include/utils/SkThreadPool.h... include/utils/SkThreadPool.h:34: * Same as Add, but doesn't swallow NULL. On 2012/10/29 19:11:09, mtklein wrote: > -> add > > :) Removed, since NULL is no longer the signal to stop.
Sign in to reply to this message.
Cool cool. I've got no opinion on whether to use NULL or fDone to kill things off. https://codereview.appspot.com/6777064/diff/7002/include/utils/SkRunnable.h File include/utils/SkRunnable.h (right): https://codereview.appspot.com/6777064/diff/7002/include/utils/SkRunnable.h#n... include/utils/SkRunnable.h:17: SK_DEFINE_DLINKEDLIST_INTERFACE(SkRunnable) This is probably not the best place to put this. This will force all SkRunnables to have extra pointers in case they ever go in a linked list. It's pretty weird to spill that over into all SkRunnables, which previously was a pretty tiny interface. Doesn't this also require all SkRunnables to be copyable now too? Why not add it like this inside SkThreadPool.h? ... private: static void* Loop(void*); struct LinkedRunnable { Runnable* fRunnable; SK_DEFINE_DLINKED_LIST_INTERFACE(LinkedRunnable); }; SkTDLinkedList<LinkedRunnable> fQueue; ... https://codereview.appspot.com/6777064/diff/7002/src/utils/SkThreadPool.cpp File src/utils/SkThreadPool.cpp (right): https://codereview.appspot.com/6777064/diff/7002/src/utils/SkThreadPool.cpp#n... src/utils/SkThreadPool.cpp:16: SkThread* thread = SkNEW_ARGS(SkThread, (&SkThreadPool::Loop, this)); Oh good call. https://codereview.appspot.com/6777064/diff/7002/src/utils/SkThreadPool.cpp#n... src/utils/SkThreadPool.cpp:24: fReady.broadcast(); Does this work? Don't we need to wrap these two lines in a fReady.lock() and .unlock()? https://codereview.appspot.com/6777064/diff/7002/src/utils/SkThreadPool.cpp#n... src/utils/SkThreadPool.cpp:42: if (pool->fDone) { If I'm thinking right this will block ~SkThreadPool() until the queue is empty. Neat. Probably deserves a comment to that effect.
Sign in to reply to this message.
After I left yesterday I realized that my desire to implement the Windows version conflicted with my earlier statement to not worry about the non-pthreads version. Sorry for the confusion :-/ Now that it seems to be working I would prefer to check in something that is mostly complete (ideally with a test, though I haven't figured out what all to cover with the test). If we must write a dummy Windows version, ideally I'd like it to only affect SkCondVar, and keep the rest independent. https://codereview.appspot.com/6777064/diff/7002/include/utils/SkRunnable.h File include/utils/SkRunnable.h (right): https://codereview.appspot.com/6777064/diff/7002/include/utils/SkRunnable.h#n... include/utils/SkRunnable.h:17: SK_DEFINE_DLINKEDLIST_INTERFACE(SkRunnable) On 2012/10/29 22:46:08, mtklein wrote: > This is probably not the best place to put this. This will force all > SkRunnables to have extra pointers in case they ever go in a linked list. It's > pretty weird to spill that over into all SkRunnables, which previously was a > pretty tiny interface. I was a little torn about this, too. I was thinking this was fine, since SkThreadPool is currently the only class that uses SkRunnable (until I realized that SkCountdown also inherits from it, and does not need the pointers). > Doesn't this also require all SkRunnables to be copyable > now too? No. > > Why not add it like this inside SkThreadPool.h? > > ... > private: > static void* Loop(void*); > > struct LinkedRunnable { > Runnable* fRunnable; > SK_DEFINE_DLINKED_LIST_INTERFACE(LinkedRunnable); > }; > > SkTDLinkedList<LinkedRunnable> fQueue; > ... The downside to that is that I need to create a bunch of wrapper objects, and new and delete them (or I can hang on to them and reuse, which would make the code bigger and messier). What if I require the client to inherit from LinkedRunnable (which in turn inherits from SkRunnable)? See next patch. https://codereview.appspot.com/6777064/diff/7002/src/utils/SkThreadPool.cpp File src/utils/SkThreadPool.cpp (right): https://codereview.appspot.com/6777064/diff/7002/src/utils/SkThreadPool.cpp#n... src/utils/SkThreadPool.cpp:24: fReady.broadcast(); On 2012/10/29 22:46:08, mtklein wrote: > Does this work? Don't we need to wrap these two lines in a fReady.lock() and > .unlock()? It seems to (probably just getting lucky), but yes, we should definitely lock and unlock. https://codereview.appspot.com/6777064/diff/7002/src/utils/SkThreadPool.cpp#n... src/utils/SkThreadPool.cpp:42: if (pool->fDone) { On 2012/10/29 22:46:08, mtklein wrote: > If I'm thinking right this will block ~SkThreadPool() until the queue is empty. > Neat. Probably deserves a comment to that effect. Not sure I follow. I think this will have the same effect as the original code. The only difference is that instead of each thread quitting when it gets a NULL Runnable in the queue, it quits once fDone is set to true and the queue is empty.
Sign in to reply to this message.
Just keep in mind that condition variables only exist since Vista, so we may need to slice the platform-specific code a little differently than you're thinking if we need to support XP, etc. The sooner something's checked in, the sooner I can help write tests. It's a particular fondness of mine to get a nice test suite covering all the code. Would be very happy to help out. https://codereview.appspot.com/6777064/diff/7002/include/utils/SkRunnable.h File include/utils/SkRunnable.h (right): https://codereview.appspot.com/6777064/diff/7002/include/utils/SkRunnable.h#n... include/utils/SkRunnable.h:17: SK_DEFINE_DLINKEDLIST_INTERFACE(SkRunnable) On 2012/10/30 15:28:54, scroggo-work wrote: > On 2012/10/29 22:46:08, mtklein wrote: > > This is probably not the best place to put this. This will force all > > SkRunnables to have extra pointers in case they ever go in a linked list. > It's > > pretty weird to spill that over into all SkRunnables, which previously was a > > pretty tiny interface. > > I was a little torn about this, too. I was thinking this was fine, since > SkThreadPool is currently the only class that uses SkRunnable (until I realized > that SkCountdown also inherits from it, and does not need the pointers). > > > Doesn't this also require all SkRunnables to be copyable > > now too? > > No. > > > > Why not add it like this inside SkThreadPool.h? > > > > ... > > private: > > static void* Loop(void*); > > > > struct LinkedRunnable { > > Runnable* fRunnable; > > SK_DEFINE_DLINKED_LIST_INTERFACE(LinkedRunnable); > > }; > > > > SkTDLinkedList<LinkedRunnable> fQueue; > > ... > > The downside to that is that I need to create a bunch of wrapper objects, and > new and delete them (or I can hang on to them and reuse, which would make the > code bigger and messier). What if I require the client to inherit from > LinkedRunnable (which in turn inherits from SkRunnable)? See next patch. I don't think this is the best way. Those extra pointers have to be allocated one way or another, so why not hide them from the API? You're forcing the users of the threadpool to give a crap about how you've implemented it if you require LinkedRunnables as input. Think of SkRunnable as an interface: the smaller the better. Maybe we should just write a queue that's not intrusive?
Sign in to reply to this message.
On 2012/10/30 20:39:06, mtklein wrote: > Just keep in mind that condition variables only exist since Vista, so we may > need to slice the platform-specific code a little differently than you're > thinking if we need to support XP, etc. Yeah, I have come to realize that will be a bit more work. Latest patch just doesn't compile the new code on windows. > > The sooner something's checked in, the sooner I can help write tests. It's a > particular fondness of mine to get a nice test suite covering all the code. > Would be very happy to help out. Great, thanks. I'll check this in so you can do that. > > https://codereview.appspot.com/6777064/diff/7002/include/utils/SkRunnable.h > File include/utils/SkRunnable.h (right): > > https://codereview.appspot.com/6777064/diff/7002/include/utils/SkRunnable.h#n... > include/utils/SkRunnable.h:17: SK_DEFINE_DLINKEDLIST_INTERFACE(SkRunnable) > On 2012/10/30 15:28:54, scroggo-work wrote: > > On 2012/10/29 22:46:08, mtklein wrote: > > > This is probably not the best place to put this. This will force all > > > SkRunnables to have extra pointers in case they ever go in a linked list. > > It's > > > pretty weird to spill that over into all SkRunnables, which previously was a > > > pretty tiny interface. > > > > I was a little torn about this, too. I was thinking this was fine, since > > SkThreadPool is currently the only class that uses SkRunnable (until I > realized > > that SkCountdown also inherits from it, and does not need the pointers). > > > > > Doesn't this also require all SkRunnables to be copyable > > > now too? > > > > No. > > > > > > Why not add it like this inside SkThreadPool.h? > > > > > > ... > > > private: > > > static void* Loop(void*); > > > > > > struct LinkedRunnable { > > > Runnable* fRunnable; > > > SK_DEFINE_DLINKED_LIST_INTERFACE(LinkedRunnable); > > > }; > > > > > > SkTDLinkedList<LinkedRunnable> fQueue; > > > ... > > > > The downside to that is that I need to create a bunch of wrapper objects, and > > new and delete them (or I can hang on to them and reuse, which would make the > > code bigger and messier). What if I require the client to inherit from > > LinkedRunnable (which in turn inherits from SkRunnable)? See next patch. > > I don't think this is the best way. Those extra pointers have to be allocated > one way or another, so why not hide them from the API? You're forcing the users > of the threadpool to give a crap about how you've implemented it if you require > LinkedRunnables as input. Think of SkRunnable as an interface: the smaller the > better. > > Maybe we should just write a queue that's not intrusive? I'll hold off on the unintrusive queue and stick with your suggestion for now.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/6777064/diff/17002/src/utils/SkThreadPool.cpp File src/utils/SkThreadPool.cpp (right): https://codereview.appspot.com/6777064/diff/17002/src/utils/SkThreadPool.cpp#... src/utils/SkThreadPool.cpp:9: #include "SkRunnable.h" R before T?
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
