SkTaskGroup::batch(fn, args, N)

SkTaskGroup::batch(fn, args, N)

Porting QuiltTask isn't important in itself; this is mostly an API feeler.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/e71cd54ed4d83310d718490d40643c35b622b9f5

[mtklein, 2014-10-29 18:39:47]

mtklein@google.com changed reviewers:
+ mtklein@google.com, reed@google.com

[mtklein, 2014-10-29 18:39:51]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-10-29 18:40:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/689673003/20001

[commit-bot: I haz the power, 2014-10-29 18:40:27]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2014-10-30 00:40 UTC

[reed1, 2014-10-29 19:57:30]

lgtm w/ questions

https://codereview.chromium.org/689673003/diff/20001/src/utils/SkTaskGroup.h
File src/utils/SkTaskGroup.h (right):

https://codereview.chromium.org/689673003/diff/20001/src/utils/SkTaskGroup.h#...
src/utils/SkTaskGroup.h:33: void batch(void (*fn)(void*), T* args, int N) {
this->batch(fn, args, N, sizeof(T)); }
1. Slightly odd to me that we don't expose the non-template version, but ok.

2. Can fn be defined to take T* instead of void*, as another convenience (avoid
casting in fn)?

[reed1, 2014-10-29 19:58:25]

I think you'll have to rebase, now that SkTaskGroup is in src/core

[commit-bot: I haz the power, 2014-10-29 19:58:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-29 19:58:39]

Failed to apply patch for src/utils/SkTaskGroup.cpp:
While running git apply --index -3 -p1;
  error: src/utils/SkTaskGroup.cpp: does not exist in index

Patch:       src/utils/SkTaskGroup.cpp
Index: src/utils/SkTaskGroup.cpp
diff --git a/src/utils/SkTaskGroup.cpp b/src/utils/SkTaskGroup.cpp
index
f1ec7f42788a198057aa0a2a4bcd916de9bb0007..aba04da61acffe88be72c679a8373fb4c4f007b9
100644
--- a/src/utils/SkTaskGroup.cpp
+++ b/src/utils/SkTaskGroup.cpp
@@ -36,6 +36,14 @@ public:
         gGlobal->add(fn, arg, pending);
     }
 
+    static void Batch(void (*fn)(void*), void* args, int N, size_t stride,
int32_t* pending) {
+        if (!gGlobal) {
+            for (int i = 0; i < N; i++) { fn((char*)args + i*stride); }
+            return;
+        }
+        gGlobal->batch(fn, args, N, stride, pending);
+    }
+
     static void Wait(int32_t* pending) {
         if (!gGlobal) {  // If we have no threads, the work must already be
done.
             SkASSERT(*pending == 0);
@@ -110,6 +118,19 @@ private:
         }
     }
 
+    void batch(void (*fn)(void*), void* arg, int N, size_t stride, int32_t*
pending) {
+        sk_atomic_add(pending, N);  // No barrier needed.
+        {
+            AutoLock lock(&fReady);
+            Work* batch = fWork.append(N);
+            for (int i = 0; i < N; i++) {
+                Work work = { fn, (char*)arg + i*stride, pending };
+                batch[i] = work;
+            }
+            fReady.broadcast();
+        }
+    }
+
     static void Loop(void* arg) {
         ThreadPool* pool = (ThreadPool*)arg;
         Work work;
@@ -154,7 +175,10 @@ SkTaskGroup::Enabler::~Enabler() {
 
 SkTaskGroup::SkTaskGroup() : fPending(0) {}
 
+void SkTaskGroup::wait()                            {
ThreadPool::Wait(&fPending); }
 void SkTaskGroup::add(SkRunnable* task)             { ThreadPool::Add(task,
&fPending); }
 void SkTaskGroup::add(void (*fn)(void*), void* arg) { ThreadPool::Add(fn, arg,
&fPending); }
-void SkTaskGroup::wait()                            {
ThreadPool::Wait(&fPending); }
+void SkTaskGroup::batch (void (*fn)(void*), void* args, int N, size_t stride) {
+    ThreadPool::Batch(fn, args, N, stride, &fPending);
+}

[mtklein, 2014-10-29 20:01:00]

https://codereview.chromium.org/689673003/diff/20001/src/utils/SkTaskGroup.h
File src/utils/SkTaskGroup.h (right):

https://codereview.chromium.org/689673003/diff/20001/src/utils/SkTaskGroup.h#...
src/utils/SkTaskGroup.h:33: void batch(void (*fn)(void*), T* args, int N) {
this->batch(fn, args, N, sizeof(T)); }
On 2014/10/29 19:57:30, reed1 wrote:
> 1. Slightly odd to me that we don't expose the non-template version, but ok.
Yeah.  Just seemed like the template version makes it harder to screw up the
stride.  We could expose the raw strided version if you like.

> 2. Can fn be defined to take T* instead of void*, as another convenience
(avoid
> casting in fn)?
No.  All the functions the threadpool deals with have to have the same type. 
When the thread picks it up, it doesn't know what type to cast the function
pointer or argument pointer to.

[mtklein, 2014-10-29 21:07:01]

> > 2. Can fn be defined to take T* instead of void*, as another convenience
> (avoid
> > casting in fn)?
> No.  All the functions the threadpool deals with have to have the same type. 
> When the thread picks it up, it doesn't know what type to cast the function
> pointer or argument pointer to.

I'm on board now.  Done.

[mtklein, 2014-10-29 21:08:21]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-10-29 21:09:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/689673003/40001

[commit-bot: I haz the power, 2014-10-29 21:17:18]

Committed patchset #3 (id:40001) as e71cd54ed4d83310d718490d40643c35b622b9f5