Run Crankshaft on a separate thread.

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=

[danno, 2012-05-22 10:32:19]

First round of comments.

BTW, in your CL description, don't include specific details about performance.
This would be enough:

Implement Crankshaft compilation on a separate thread. 

Enabled by the "--concurrent-crankshaft" flag (set to "false" by default, hot
functions are put on a compilation queue and crankshaft'ed on a separate
thread..

http://codereview.chromium.org/10417010/diff/1/src/SConscript
File src/SConscript (right):

http://codereview.chromium.org/10417010/diff/1/src/SConscript#newcode57
src/SConscript:57: crankshaft-thread.cc
Don't refer to crankshaft if filenames/variables. Use "optimizing compiler", so
here "optimizing-compiler-thread.cc"

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#newcode39
src/api.cc:39: #include "crankshaft-thread.h"
crankshaft -> optimizing_compiler

http://codereview.chromium.org/10417010/diff/1/src/api.cc#newcode5397
src/api.cc:5397: isolate->TearDown();
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?

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#newco...
src/crankshaft-thread.cc:42: for (std::set<Isolate*>::iterator i =
deleted_isolates->begin(),
We don't use STL in V8, you'll have to find another way to store the set. Better
yet, get rid of this deferred disposal (see below).

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.cc#newco...
src/crankshaft-thread.cc:62: loop_lock_->Unlock();
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...

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.cc#newco...
src/crankshaft-thread.cc:83: Acquire_Store(&isolate_,
reinterpret_cast<AtomicWord>(isolate));
I think you can get rid of both isolate_ and loop_lock if you follow my
suggestion above.

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.cc#newco...
src/crankshaft-thread.cc:99: isolate->~Isolate();
What happens if two functions from the isolate are queued? You'll dispose it
twice, and access the memory from it after the first dispose is done. I think it
is better to have a queue lock, and when a isolate is disposed, flush the queue
of all of the isolates. You will need to be able to deal with dequeueing empty
elements, but that's probably not hard.

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.cc#newco...
src/crankshaft-thread.cc:108: FreeDeletedIsolates(&deleted_isolates);
How big are the isolates? Can we really afford to only dispose them when the
compilation thread exits?

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.cc#newco...
src/crankshaft-thread.cc:120: }
Don't you have to do this after the Enter to ensure that nobody steps on the
stack guard you set up?

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.cc#newco...
src/crankshaft-thread.cc:132: while (CompileJSFunction(isolate,
This won't work. You will have to create a handle version of CompileJSFunction
in factory.cc that does the GC->retry cycle. This logic will just end up in an
endless loop.

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.cc#newco...
src/crankshaft-thread.cc:191: AddElement(element);
Why do you need the explicit dummy element? You set stop_loop_ already. Instead
of AddElement, just queue_semaphore_->Signal() ?

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.cc#newco...
src/crankshaft-thread.cc:204: ElementsAccessor::TearDown();
Why are these here? Shouldn't they be in V8 teardown (they are not specific to
the compilation thread).

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.cc#newco...
src/crankshaft-thread.cc:210: functions_compiled_);
use v8-counters for this

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.cc#newco...
src/crankshaft-thread.cc:220: PrintF(stderr, "    > Starting Crankshaft
Thread\n");
Remove the print, please

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.cc#newco...
src/crankshaft-thread.cc:221: #endif
Remove the print, please

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#newcode28
src/crankshaft-thread.h:28: #ifndef CRANKSHAFT_THREAD_H_
crankshaft -> concurrent_compiler

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.h#newcode41
src/crankshaft-thread.h:41: class CrankshaftThread {
This isn't a thread, is it? Perhaps ConcurrentCompiler is better?
Also, since this wrapper class doesn't really have a lot of meat, perhaps you
can move some of it to the isolate? eg:

isolate->QueueFunctionToCompile(js_function);

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.h#newcode44
src/crankshaft-thread.h:44: static void StopThread();
The above methods should be on the isolate, it seems strange that they are
static.

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.h#newcode45
src/crankshaft-thread.h:45: static void AddJob(i::Isolate *isolate,
i::Handle<JSFunction> function);
How about QueueFunctionToCompile instead of AddJob? Also, this should be a
method on the isolate (no statics, please).

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.h#newcode48
src/crankshaft-thread.h:48: struct WorkElement {
Perhaps "PendingCompilation" is more descriptive?

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.h#newcode51
src/crankshaft-thread.h:51: bool dummy;
Instead of having the explicit dummy field, why not just use a sentinel value
for the isolate (null) or function (undefined?)

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.h#newcode54
src/crankshaft-thread.h:54: typedef UnboundQueue<WorkElement> WorkQueue;
CompilationQueue

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.h#newcode56
src/crankshaft-thread.h:56: class WorkerThread : public Thread {
CompilerThread

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.h#newcode59
src/crankshaft-thread.h:59: queue_semaphore_ = OS::CreateSemaphore(0);
Where does this get disposed?

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.h#newcode61
src/crankshaft-thread.h:61: queue_lock_ = OS::CreateMutex();
Where to loop_lock_ and queue_lock_ get disposed?

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.h#newcode69
src/crankshaft-thread.h:69: void AddElement(const WorkElement &);
QueuePendingCompilation

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.h#newcode72
src/crankshaft-thread.h:72: WorkQueue work_queue_;
compilation_queue_

http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.h#newcode87
src/crankshaft-thread.h:87: };
nit: add line break

http://codereview.chromium.org/10417010/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

http://codereview.chromium.org/10417010/diff/1/src/flag-definitions.h#newcode480
src/flag-definitions.h:480: DEFINE_bool(concurrent_crankshaft, false, "run
crankshaft on a thread of its "
There is already a "Flags for Crankshaft section" in flag-definitions, please
put this flag there.

http://codereview.chromium.org/10417010/diff/1/src/isolate.h
File src/isolate.h (right):

http://codereview.chromium.org/10417010/diff/1/src/isolate.h#newcode1135
src/isolate.h:1135: // edge-case, when we call Exit(false)
Can you be more explicit why this case happens?

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#newcod...
src/runtime-profiler.cc:135: if (FLAG_concurrent_crankshaft &&
!(isolate_->context() == NULL ||
I don't see why this is necessary, can you explain in more detail?

http://codereview.chromium.org/10417010/diff/1/src/v8.cc
File src/v8.cc (right):

http://codereview.chromium.org/10417010/diff/1/src/v8.cc#newcode122
src/v8.cc:122: ElementsAccessor::TearDown();
Yikes, this tear down work should always happen here, even when the crankshaft
thread is active... even if it requires some sort of synchronization.

http://codereview.chromium.org/10417010/diff/1/src/v8.cc#newcode268
src/v8.cc:268: if (i::FLAG_concurrent_crankshaft)
nit: Always use { } even around single line statements:

if (i::FLAG_concurrent_crankshaft) {
  i::CrankshaftThread::StartThread();
}

[sanjoy, 2012-05-22 11:38:55]

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#newco...
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#newco...
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#newco...
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#newcod...
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.

[fschneider, 2012-05-24 18:05:17]

On 2012/05/24 11:13:12, sanjoy wrote:

Just an idea: Wouldn't it be easier and more productive to do this effort on an
experimental/ branch? I think it would.

[danno, 2012-05-25 07:29:38]

I agree. Sanjoy, let's create a branch for the more radical changes like this
one.  BTW, some of your changes, like the explicit Zone-passing work, should
still be landed on bleeding_edge, because there is already benefit in this for
the existing code base without parallel compilation.

I'll comment on the CL itself later today.

[chunyang.stock, 2012-07-13 08:24:32]

I have the same idea about 3 months ago. I measured the total optimized
compilation time of some famous test cases. Generally speaking, on Desktop the
optimized compilation time occupied about 2% ~ 3%. Some special case will
occupied more then 10%. And on other slow devices such as phone and tablet, it's
about 5% ~ 6%. 

I did one prototype I have some performance gain. And I prepared to continue to
optimize and propose the implementation but I found this subject is already
being implemented.

Most of my implementation is the same as Sanjoy's code. Some different points
are the following:
  1, I use thread local data to manage the data in Isolate which are shared by
the main thread and the compiler thread such as HandleScope, the Zone etc.
  2, I implemented the parallel compilation support for OSR. 

Danno suggested to create one branch for this subject. So May I help to work at
project? I have 3 years working experience on JVM  JIT compiler and another 2
years experience on another JIT compiler for XSLT language. 
I will contribute all my knowledge if possible. 

thanks.