https://codereview.chromium.org/104583003/diff/40001/src/libplatform/default-platform.cc
File src/libplatform/default-platform.cc (right):
https://codereview.chromium.org/104583003/diff/40001/src/libplatform/default-platform.cc#newcode62
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-platform.cc#newcode87
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-platform.h
File src/libplatform/default-platform.h (right):
https://codereview.chromium.org/104583003/diff/40001/src/libplatform/default-platform.h#newcode63
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-queue.cc
File src/libplatform/task-queue.cc (right):
https://codereview.chromium.org/104583003/diff/40001/src/libplatform/task-queue.cc#newcode64
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-queue.h
File src/libplatform/task-queue.h (right):
https://codereview.chromium.org/104583003/diff/40001/src/libplatform/task-queue.h#newcode31
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-libplatform-worker-thread.cc
File test/cctest/test-libplatform-worker-thread.cc (right):
https://codereview.chromium.org/104583003/diff/40001/test/cctest/test-libplatform-worker-thread.cc#newcode57
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-libplatform.h
File test/cctest/test-libplatform.h (right):
https://codereview.chromium.org/104583003/diff/40001/test/cctest/test-libplatform.h#newcode51
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.
https://codereview.chromium.org/104583003/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
