Reviewers: danno,
Message:
I've responded to some of the comments. I'll upload a fixed CL soon.
http://codereview.chromium.org/10417010/diff/1/src/api.cc
File src/api.cc (right):
http://codereview.chromium.org/10417010/diff/1/src/api.cc#newcode5397
src/api.cc:5397: isolate->TearDown();
On 2012/05/22 10:32:19, danno wrote:
How do you ensure that the compiler thread properly terminates and
cleans itself
up (i.e. doesn't do any further compilation) after the TearDown is
complete?
V8::TearDown stops the compiler thread.
http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.cc
File src/crankshaft-thread.cc (right):
http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.cc#newcode62
src/crankshaft-thread.cc:62: loop_lock_->Unlock();
On 2012/05/22 10:32:19, danno wrote:
I don't think you need the loop lock at all. Just dequeue elements
over and over
again.
queue_semaphore_->Wait();
bool to_stop = Release_Load(&stop_loop_);
if (to_stop) return;
{
Locker locker(&queue_lock);
if (work_queue.IsEmpty()) continue;
WorkElement w_element;
work_queue_.Dequeue(&w_element);
}
// compile w_element...
I needed a place in the code where I'm assured isolate_ is set to the
Isolate currently being compiled under, so that I can unlock that
isolate from StopThread if it is locked by the current thread.
Isolates have to be locked before they're deleted to prevent the
optimizing compiler thread from compiling functions in an isolate while
it is being torn down. The above deadlocked situation arises when a
thread destroys the last isolate and tries to stop the compiler queue.
But I think I can still do without the loop locking, I'll try to be more
clever in the second iteration. Things should become simpler once the
optimizing compiler thread starts doing only the parts it can do in
parallel.
http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.cc#newcode120
src/crankshaft-thread.cc:120: }
On 2012/05/22 10:32:19, danno wrote:
Don't you have to do this after the Enter to ensure that nobody steps
on the
stack guard you set up?
I don't think so, I've already acquired the isolate lock by now. When
running with --concurrent_crankshaft, Enter()ing and Exit()ing isolates
always acquire / release the lock.
http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.cc#newcode191
src/crankshaft-thread.cc:191: AddElement(element);
On 2012/05/22 10:32:19, danno wrote:
Why do you need the explicit dummy element? You set stop_loop_
already. Instead
of AddElement, just queue_semaphore_->Signal() ?
Right, it is not needed. I'll remove it in the next iteration.
http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.h
File src/crankshaft-thread.h (right):
http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.h#newcode61
src/crankshaft-thread.h:61: queue_lock_ = OS::CreateMutex();
On 2012/05/22 10:32:19, danno wrote:
Where to loop_lock_ and queue_lock_ get disposed?
They don't. Will fix this in the next iteration.
http://codereview.chromium.org/10417010/diff/1/src/runtime-profiler.cc
File src/runtime-profiler.cc (right):
http://codereview.chromium.org/10417010/diff/1/src/runtime-profiler.cc#newcode135
src/runtime-profiler.cc:135: if (FLAG_concurrent_crankshaft &&
!(isolate_->context() == NULL ||
On 2012/05/22 10:32:19, danno wrote:
I don't see why this is necessary, can you explain in more detail?
Sometimes this function is called with the context slot set to a
non-context. You can check this by putting ASSERT(!isolate_->context()
|| isolate_->context()->IsContext()); here and running the unit tests in
debug mode.
I'll put in a more detailed comment in the second iteration.
Description:
Run Crankshaft on a separate thread.
Hot functions are put on a compilation queue and crankshaft'ed on a
separate thread. Slows down performance by three times when turned on
(--concurrent_crankshaft) but benchmark numbers aren't affected when
the option is turned off.
BUG=
TEST=
Please review this at http://codereview.chromium.org/10417010/
SVN Base: https://chromiumcodereview.appspot.com/10387157
Affected files:
M src/SConscript
M src/api.cc
A + src/crankshaft-thread.h
A src/crankshaft-thread.cc
M src/d8.cc
M src/factory.cc
M src/flag-definitions.h
M src/isolate.h
M src/isolate.cc
M src/objects.h
M src/objects.cc
M src/runtime-profiler.cc
M src/runtime.h
M src/runtime.cc
M src/v8.cc
M src/v8threads.cc
M test/cctest/test-debug.cc
M test/cctest/test-deoptimization.cc
M test/cctest/test-heap.cc
M test/cctest/test-mark-compact.cc
M test/cctest/test-threads.cc
M test/mjsunit/assert-opt-and-deopt.js
M test/mjsunit/regress/regress-1118.js
M tools/gyp/v8.gyp
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
