[platform] Implement a worker pool

[platform] Implement a worker pool

We don't use the worker pool yet, however, there are tests. Yay. The
next step is to use the worker pool for parallel sweeping.

I've also started to move the platform related files into a sub
directory. The goal is to eventually build all the platform stuff as
a separate library which is used by d8 and cctest (and other embedders
that wish to use the default implementation) but not by chromium.

BUG=v8:3015
R=hpayer@chromium.org, svenpanne@chromium.org
LOG=n

Committed: https://code.google.com/p/v8/source/detail?r=18380

[jochen (gone - plz use gerrit), 2013-12-05 08:55:04]

+svenpanne

[Sven Panne, 2013-12-05 10:04:11]

First round of comments...

https://codereview.chromium.org/104583003/diff/1/src/libplatform/default-plat...
File src/libplatform/default-platform.cc (right):

https://codereview.chromium.org/104583003/diff/1/src/libplatform/default-plat...
src/libplatform/default-platform.cc:88: thread_pool_size_ =
Max(Min(thread_pool_size_, 4), 1);
Wouldn't it be better to do some sanity checks/clamping in
DefaultPlatform::SetThreadPoolSize instead? Furhtermore, it might be nice to
have a flag for the hardcoded limit of 4.

https://codereview.chromium.org/104583003/diff/1/src/libplatform/default-plat...
File src/libplatform/default-platform.h (right):

https://codereview.chromium.org/104583003/diff/1/src/libplatform/default-plat...
src/libplatform/default-platform.h:60: // TODO(jochen): This shouldn't be a
static.
What's the reason for "static"? Wouldn't it be easier to have a
std::list<WorkerThread*> here? Furthermore, the WorkerThreads are owned by the
list, so it might be nice to have a std::list<WorkerThread>, but I guess we
would have to do some massaging to the WorkerThread class hierarchy to make the
threads auto-join on destruction.

[jochen (gone - plz use gerrit), 2013-12-05 12:58:52]

https://codereview.chromium.org/104583003/diff/1/src/libplatform/default-plat...
File src/libplatform/default-platform.cc (right):

https://codereview.chromium.org/104583003/diff/1/src/libplatform/default-plat...
src/libplatform/default-platform.cc:88: thread_pool_size_ =
Max(Min(thread_pool_size_, 4), 1);
On 2013/12/05 10:04:11, Sven Panne wrote:
> Wouldn't it be better to do some sanity checks/clamping in
> DefaultPlatform::SetThreadPoolSize instead?

We'd need to do the clamping and sanity check here anyway, in case
SetThreadPoolSize was never invoked.


> Furhtermore, it might be nice to
> have a flag for the hardcoded limit of 4.

I dunno. I'd add the flag when it's needed.

https://codereview.chromium.org/104583003/diff/1/src/libplatform/default-plat...
File src/libplatform/default-platform.h (right):

https://codereview.chromium.org/104583003/diff/1/src/libplatform/default-plat...
src/libplatform/default-platform.h:60: // TODO(jochen): This shouldn't be a
static.
On 2013/12/05 10:04:11, Sven Panne wrote:
> What's the reason for "static"? Wouldn't it be easier to have a
> std::list<WorkerThread*> here? Furthermore, the WorkerThreads are owned by the
> list, so it might be nice to have a std::list<WorkerThread>, but I guess we
> would have to do some massaging to the WorkerThread class hierarchy to make
the
> threads auto-join on destruction.

The reason for static is that currently v8 magically creates the platform
internally.

When the embedder creates the platform, it can just be a method.

The way it is now, we create default platform when initializing the default
isolate, however, the resource constraints are set before the default isolate is
initialized.

I've changed worker thread to auto-join on destruction.

Using list<WorkerThread> would mean that they need to be copyable etc..

[Sven Panne, 2013-12-05 13:12:30]

LGTM with a nit.

https://codereview.chromium.org/104583003/diff/20001/src/libplatform/default-...
File src/libplatform/default-platform.cc (right):

https://codereview.chromium.org/104583003/diff/20001/src/libplatform/default-...
src/libplatform/default-platform.cc:87: thread_pool_size_ =
Max(Min(thread_pool_size_, 4), 1);
Either make it a v8 flag or make it at least a constant in DefaultPlatform
(kMaxPoolSize). Burying some magic number deep in the implementation is not
nice.

[Hannes Payer (out of office), 2013-12-09 18:50:51]

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/default-...
File src/libplatform/default-platform.cc (right):

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/default-...
src/libplatform/default-platform.cc:62: ASSERT(thread_pool_size >= 0);
What about: void DefaultPlatform::SetThreadPoolSize(int thread_pool_size=0)
and setting thread_pool_size_ to CPU::NumberOfProcessorsOnline() if
thread_pool_size==0.

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/default-...
src/libplatform/default-platform.cc:87: thread_pool_size_ =
Max(Min(thread_pool_size_, kMaxThreadPoolSize), 1);
I think this should be handled by SetThreadPoolSize.

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/default-...
File src/libplatform/default-platform.h (right):

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/default-...
src/libplatform/default-platform.h:63: static int thread_pool_size_;
I don't think we can have static members here. This may be a problem when we
have multiple isolates per process?

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/task-que...
File src/libplatform/task-queue.cc (right):

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/task-que...
src/libplatform/task-queue.cc:64: process_queue_semaphore_.Signal();
Why do you need that signal? The thread that terminates the task queue is not
going to wait.

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/task-que...
File src/libplatform/task-queue.h (right):

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/task-que...
src/libplatform/task-queue.h:31: #include <queue>
Uh, funky. Not sure if we wanna start using that in v8.

https://codereview.chromium.org/104583003/diff/40001/test/cctest/test-libplat...
File test/cctest/test-libplatform-worker-thread.cc (right):

https://codereview.chromium.org/104583003/diff/40001/test/cctest/test-libplat...
test/cctest/test-libplatform-worker-thread.cc:57: // TaskQueue ASSERTs that it
is empty in its dtor.
Can we call it destructor?

https://codereview.chromium.org/104583003/diff/40001/test/cctest/test-libplat...
File test/cctest/test-libplatform.h (right):

https://codereview.chromium.org/104583003/diff/40001/test/cctest/test-libplat...
test/cctest/test-libplatform.h:51: --*task_counter_;
I guess this operation has to be done atomic, otherwise a race may happen.

[jochen (gone - plz use gerrit), 2013-12-09 19:04:31]

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/default-...
File src/libplatform/default-platform.cc (right):

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/default-...
src/libplatform/default-platform.cc:62: ASSERT(thread_pool_size >= 0);
On 2013/12/09 18:50:51, Hannes Payer wrote:
> What about: void DefaultPlatform::SetThreadPoolSize(int thread_pool_size=0)
> and setting thread_pool_size_ to CPU::NumberOfProcessorsOnline() if
> thread_pool_size==0.

default parameters are forbidden by the coding style :)

CPU::NumberOfProccessorsOnline() is already used as fallback. However, that
doesn't work in production (which is why we get the number of
ResourceConstraints - see api.cc)

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/default-...
src/libplatform/default-platform.cc:87: thread_pool_size_ =
Max(Min(thread_pool_size_, kMaxThreadPoolSize), 1);
On 2013/12/09 18:50:51, Hannes Payer wrote:
> I think this should be handled by SetThreadPoolSize.

I would have to duplicate this here for the case that SetThreadPoolSize is never
invoked. So why bother?

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/default-...
File src/libplatform/default-platform.h (right):

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/default-...
src/libplatform/default-platform.h:63: static int thread_pool_size_;
On 2013/12/09 18:50:51, Hannes Payer wrote:
> I don't think we can have static members here. This may be a problem when we
> have multiple isolates per process?

there's only one default platform per process.

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/task-que...
File src/libplatform/task-queue.cc (right):

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/task-que...
src/libplatform/task-queue.cc:64: process_queue_semaphore_.Signal();
On 2013/12/09 18:50:51, Hannes Payer wrote:
> Why do you need that signal? The thread that terminates the task queue is not
> going to wait.

There can be multiple threads waiting on the semaphore and all need to be woken
up.

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/task-que...
File src/libplatform/task-queue.h (right):

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/task-que...
src/libplatform/task-queue.h:31: #include <queue>
On 2013/12/09 18:50:51, Hannes Payer wrote:
> Uh, funky. Not sure if we wanna start using that in v8.

We already use several other STL containers.

https://codereview.chromium.org/104583003/diff/40001/test/cctest/test-libplat...
File test/cctest/test-libplatform-worker-thread.cc (right):

https://codereview.chromium.org/104583003/diff/40001/test/cctest/test-libplat...
test/cctest/test-libplatform-worker-thread.cc:57: // TaskQueue ASSERTs that it
is empty in its dtor.
On 2013/12/09 18:50:51, Hannes Payer wrote:
> Can we call it destructor?

sure

https://codereview.chromium.org/104583003/diff/40001/test/cctest/test-libplat...
File test/cctest/test-libplatform.h (right):

https://codereview.chromium.org/104583003/diff/40001/test/cctest/test-libplat...
test/cctest/test-libplatform.h:51: --*task_counter_;
On 2013/12/09 18:50:51, Hannes Payer wrote:
> I guess this operation has to be done atomic, otherwise a race may happen.

good point. Will update.

[jochen (gone - plz use gerrit), 2013-12-10 09:07:12]

updated, PTAL

[Sven Panne, 2013-12-10 11:35:52]

LGTM (still) with a nit

https://codereview.chromium.org/104583003/diff/100001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/104583003/diff/100001/src/api.cc#newcode633
src/api.cc:633: 
Nit: Spurious change?

[Hannes Payer (out of office), 2013-12-11 12:07:26]

LGTM, just minor nits.

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/default-...
File src/libplatform/default-platform.cc (right):

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/default-...
src/libplatform/default-platform.cc:62: ASSERT(thread_pool_size >= 0);
On 2013/12/09 19:04:31, jochen wrote:
> On 2013/12/09 18:50:51, Hannes Payer wrote:
> > What about: void DefaultPlatform::SetThreadPoolSize(int thread_pool_size=0)
> > and setting thread_pool_size_ to CPU::NumberOfProcessorsOnline() if
> > thread_pool_size==0.
> 
> default parameters are forbidden by the coding style :)
> 
> CPU::NumberOfProccessorsOnline() is already used as fallback. However, that
> doesn't work in production (which is why we get the number of
> ResourceConstraints - see api.cc)

OK, fine with me.

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/default-...
src/libplatform/default-platform.cc:87: thread_pool_size_ =
Max(Min(thread_pool_size_, kMaxThreadPoolSize), 1);
On 2013/12/09 19:04:31, jochen wrote:
> On 2013/12/09 18:50:51, Hannes Payer wrote:
> > I think this should be handled by SetThreadPoolSize.
> 
> I would have to duplicate this here for the case that SetThreadPoolSize is
never
> invoked. So why bother?

OK, you could also invoke SetThreadPool size from here, but fine.

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/default-...
File src/libplatform/default-platform.h (right):

https://codereview.chromium.org/104583003/diff/40001/src/libplatform/default-...
src/libplatform/default-platform.h:63: static int thread_pool_size_;
On 2013/12/09 19:04:31, jochen wrote:
> On 2013/12/09 18:50:51, Hannes Payer wrote:
> > I don't think we can have static members here. This may be a problem when we
> > have multiple isolates per process?
> 
> there's only one default platform per process.

As discussed offline, TSAN will not like that in a build with two isolates per
process.

https://codereview.chromium.org/104583003/diff/120001/src/libplatform/task-qu...
File src/libplatform/task-queue.h (right):

https://codereview.chromium.org/104583003/diff/120001/src/libplatform/task-qu...
src/libplatform/task-queue.h:49: // Appends a task to the queue. Takes
ownership.
You mean the queue takes ownership, right. Can we make that comment more
explicit?

https://codereview.chromium.org/104583003/diff/120001/test/cctest/test-libpla...
File test/cctest/test-libplatform.h (right):

https://codereview.chromium.org/104583003/diff/120001/test/cctest/test-libpla...
test/cctest/test-libplatform.h:63: 
Can we have a newline here?

https://codereview.chromium.org/104583003/diff/120001/test/cctest/test-libpla...
test/cctest/test-libplatform.h:71: }
Can we have a newline here?

https://codereview.chromium.org/104583003/diff/120001/test/cctest/test-libpla...
test/cctest/test-libplatform.h:75: }
Can we have a newline here?

[jochen (gone - plz use gerrit), 2013-12-11 13:17:07]

all done

[jochen (gone - plz use gerrit), 2013-12-20 07:53:20]

Committed patchset #8 manually as r18380 (presubmit successful).