Figuring out how to integrate this threadpool into Skia.

I wrote up a thread pool blindly ignorant of the Skia codebase (here under threads/) and am now trying to figure out how to express it in Skia.  I have some guesses but I think it'd be helpful to get early feedback:

 - Instead of vector<T> I should be using SkTArray<T> or SkTDArray<T>.
 - Instead of queue<T> I should be using SkLinkedList<T>.
 - Where I write CondVar I suspect I want to abstract out PthreadEvent/HANDLE into a platform-dependent SkCondVar class.
 - Seems like we use pthreads everywhere but Windows, which is terra incognita to me.  I haven't even looked at Windows yet, but the instructions on skiadocs look doable.
 - I think I want to move Runnable and ThreadPool into SkThreadUtils.

Does it seem like I'm on the right track?

I've also added in a small tweak and script I've been using to see how the pictures benchmarks varies when run with various numbers of threads.

[mtklein, 2012-10-23 18:39:32]

Hiya.  Can you help me integrating this into Skia?  It's a rather different
codebase than I'm used to.

[Leon, 2012-10-23 19:58:51]

Looks like the right track to me. It seems you left out PictureRenderer? I'm
curious to know what it looks like. Looking at main.cc, it looks to me like you
require the client to be in charge of the Countdown. Do you think we can make
that a part of ThreadPool?

Our styleguide is quite a bit different from google3. It can be found at
https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...
(it is incomplete, but that will give you an idea).

> - Instead of vector<T> I should be using SkTArray<T> or SkTDArray<T>.

Correct. I typically use an SkTDArray of pointers.

> - Instead of queue<T> I should be using SkLinkedList<T>.

Is there an SkLinkedList? You can use SkTDArray, which has methods to be treated
like a queue.

> - Where I write CondVar I suspect I want to abstract out PthreadEvent/HANDLE
> into a platform-dependent SkCondVar class.

Agreed. I also think that ThreadPool should be renamed SkThreadPool and made
platform-independent.

> - Seems like we use pthreads everywhere but Windows, which is terra incognita
> to me.  I haven't even looked at Windows yet, but the instructions on skiadocs
> look doable.

Let me know if you have any problems.

> - I think I want to move Runnable and ThreadPool into SkThreadUtils.

That sounds good to me.

[mtklein, 2012-10-23 20:17:27]

On 2012/10/23 19:58:51, scroggo-work wrote:
> Looks like the right track to me. It seems you left out PictureRenderer? I'm
> curious to know what it looks like.
Yeah, you caught me: I was delaying actually hooking this up until I could get
it properly Skiaized.

> Looking at main.cc, it looks to me like you
> require the client to be in charge of the Countdown. Do you think we can make
> that a part of ThreadPool?
What sort of API did you have in mind?  We can do a lot of things, but in my
experience things tend to be easier to keep orthogonal when the threadpool is
dumb.

> Our styleguide is quite a bit different from google3. It can be found at
>
https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...
> (it is incomplete, but that will give you an idea).
> 
I anticipate that every character is going to be different. :)

> > - Instead of vector<T> I should be using SkTArray<T> or SkTDArray<T>.
> 
> Correct. I typically use an SkTDArray of pointers.
Cool, thanks.

> 
> > - Instead of queue<T> I should be using SkLinkedList<T>.
> 
> Is there an SkLinkedList? You can use SkTDArray, which has methods to be
treated
> like a queue.
There's an SkTDLinkedList.  For SkTDArray, it looks like it's written the usual
way for O(1) amortized back inserts (append, push) and removal (pop), but
pushing onto the front (prepend), and removing from the front (remove(0)) look
O(N) to me (because of memmove).  Am I reading it wrong?


> > - Where I write CondVar I suspect I want to abstract out PthreadEvent/HANDLE
> > into a platform-dependent SkCondVar class.
> 
> Agreed. I also think that ThreadPool should be renamed SkThreadPool and made
> platform-independent.
Yep.  Should be that if we have SkThread and SkCondVar implemented for each
platform, SkThreadPool won't need any platform-specific treatment.

> 
> > - Seems like we use pthreads everywhere but Windows, which is terra
incognita
> > to me.  I haven't even looked at Windows yet, but the instructions on
skiadocs
> > look doable.
> 
> Let me know if you have any problems.
> 
> > - I think I want to move Runnable and ThreadPool into SkThreadUtils.
> 
> That sounds good to me.

[Leon, 2012-10-23 20:34:12]

>  Looking at main.cc, it looks to me like you
>> require the client to be in charge of the Countdown. Do you think we
>>
> can make
>
>> that a part of ThreadPool?
>>
> What sort of API did you have in mind?  We can do a lot of things, but
> in my experience things tend to be easier to keep orthogonal when the
> threadpool is dumb.


I'm not sure exactly what the API should be. I imagined that a common use
case would be for the thread pool to wait, but since we currently only have
one use case, it's just speculation. I am fine with leaving it as is for
now.

>
>
>  Our styleguide is quite a bit different from google3. It can be found
>>
> at
>
> https://sites.google.com/site/**skiadocs/developer-**
>
documentation/contributing-**code/coding-style-guidelines<https://sites.google.com/site/skiadocs/developer-documentation/contributing-code/coding-style-guidelines>
>
>> (it is incomplete, but that will give you an idea).
>>
>
> I anticipate that every character is going to be different. :)
>
>
>  > - Instead of vector<T> I should be using SkTArray<T> or
>>
> SkTDArray<T>.
>
>  Correct. I typically use an SkTDArray of pointers.
>>
> Cool, thanks.
>
>
>
>  > - Instead of queue<T> I should be using SkLinkedList<T>.
>>
>
>  Is there an SkLinkedList? You can use SkTDArray, which has methods to
>>
> be treated
>
>> like a queue.
>>
> There's an SkTDLinkedList.  For SkTDArray, it looks like it's written
> the usual way for O(1) amortized back inserts (append, push) and removal
> (pop), but pushing onto the front (prepend), and removing from the front
> (remove(0)) look O(N) to me (because of memmove).  Am I reading it
> wrong?


No, that sounds right to me. You're not currently using prepend, but I
think SkTDLinkedList would work fine as well.

>
>
>
>  > - Where I write CondVar I suspect I want to abstract out
>>
> PthreadEvent/HANDLE
>
>> > into a platform-dependent SkCondVar class.
>>
>
>  Agreed. I also think that ThreadPool should be renamed SkThreadPool
>>
> and made
>
>> platform-independent.
>>
> Yep.  Should be that if we have SkThread and SkCondVar implemented for
> each platform, SkThreadPool won't need any platform-specific treatment.
>
>
>
>  > - Seems like we use pthreads everywhere but Windows, which is terra
>>
> incognita
>
>> > to me.  I haven't even looked at Windows yet, but the instructions
>>
> on skiadocs
>
>> > look doable.
>>
>
>  Let me know if you have any problems.
>>
>
>  > - I think I want to move Runnable and ThreadPool into SkThreadUtils.
>>
>
>  That sounds good to me.
>>
>
>
>
>
https://codereview.appspot.**com/6755043/<https://codereview.appspot.com/6755...
>



-- 
Leon Scroggins III
scroggo@google.com

[Leon, 2012-10-26 19:43:59]

https://codereview.appspot.com/6755043/diff/2001/threads/countdown.cc
File threads/countdown.cc (right):

https://codereview.appspot.com/6755043/diff/2001/threads/countdown.cc#newcode6
threads/countdown.cc:6: ready_.Lock();
Would it be faster (and just as safe) to use atomic decrement to avoid some
locking and signaling?
Run() {
  if (sk_atomic_dec(&n_) == 1) { // sk_atomic_dec subtracts one and returns the
original value
    ready_.Lock();
    ready_.Signal();
    ready_.Unlock();
  }
}

Wait() {
  ready_.Lock();
  while (n_ > 0) {
    ready_.Wait();
  }
  ready_.Unlock();
}

https://codereview.appspot.com/6755043/diff/2001/threads/countdown.cc#newcode16
threads/countdown.cc:16: }
Doesn't Wait() lock once it resumes? Shouldn't we call

ready_.Unlock()

here?

https://codereview.appspot.com/6755043/diff/2001/threads/threadpool.h
File threads/threadpool.h (right):

https://codereview.appspot.com/6755043/diff/2001/threads/threadpool.h#newcode22
threads/threadpool.h:22: void Add(Runnable*);
I want each thread to be able to call the function to create the TLS font data.
It might be nice to have a way to ensure that happens. Maybe something like
AddOnAllThreads? Or maybe a Runnable could be passed to the constructor to run
once at the beginning.

(Alternatively, I can call the function before each draw; it won't break
anything but might slow things down.)