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#newcode42
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#newcode62
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#newcode83
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#newcode99
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#newcode108
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#newcode120
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#newcode132
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#newcode191
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#newcode204
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#newcode210
src/crankshaft-thread.cc:210: functions_compiled_);
use v8-counters for this
http://codereview.chromium.org/10417010/diff/1/src/crankshaft-thread.cc#newcode220
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#newcode221
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#newcode135
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();
}
http://codereview.chromium.org/10417010/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
