Optimize functions on a second thread.

Optimize functions on a second thread.


BUG=
TEST=

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

[sanjoy, 2012-07-18 22:57:37]

Parallel recompilation is turned off by default.  Please use the
--parallel-recompilation flag.

[Yang, 2012-07-19 12:47:30]

Did a first pass. I'll do a second pass but here's the first round of comments.

http://codereview.chromium.org/10807024/diff/2001/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/10807024/diff/2001/src/compiler.cc#newcode875
src/compiler.cc:875: // If crankshaft succeeded, install the optimized code else
install
"If crankshaft succeeded, install the optimized code. Otherwise install the
unoptimized code."

http://codereview.chromium.org/10807024/diff/2001/src/execution.cc
File src/execution.cc (right):

http://codereview.chromium.org/10807024/diff/2001/src/execution.cc#newcode966
src/execution.cc:966: 
Stray edit?

http://codereview.chromium.org/10807024/diff/2001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/10807024/diff/2001/src/heap.cc#newcode6252
src/heap.cc:6252: if (FLAG_parallel_recompilation) delete relocation_mutex_;
I think you can omit checking for the flag since deleting NULL is safe.

http://codereview.chromium.org/10807024/diff/2001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (left):

http://codereview.chromium.org/10807024/diff/2001/src/hydrogen-instructions.h...
src/hydrogen-instructions.h:2292: ASSERT(!HEAP->IsAllocationAllowed());
It would be great if we could distinguish between the threads to still check
that the optimizing compile thread does not allocate. Since we never allocate
anything in the optimizing compile thread, why not have every place that checks
for allocation_allowed_ to also check that the thread is not the optimizing
compile thread?

http://codereview.chromium.org/10807024/diff/2001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

http://codereview.chromium.org/10807024/diff/2001/src/ia32/builtins-ia32.cc#n...
src/ia32/builtins-ia32.cc:80: __ add(eax, Immediate(Code::kHeaderSize -
kHeapObjectTag));
Use the lea instruction and FieldOperand would be easier to read.

http://codereview.chromium.org/10807024/diff/2001/src/isolate.h
File src/isolate.h (left):

http://codereview.chromium.org/10807024/diff/2001/src/isolate.h#oldcode764
src/isolate.h:764: 
Stray edit?

http://codereview.chromium.org/10807024/diff/2001/src/optimizing-compiler-thr...
File src/optimizing-compiler-thread.cc (right):

http://codereview.chromium.org/10807024/diff/2001/src/optimizing-compiler-thr...
src/optimizing-compiler-thread.cc:81: OptimizingCompiler* compiler = NULL;
Maybe OptimizingCompileTask would be a better name than OptimizingCompiler.

http://codereview.chromium.org/10807024/diff/2001/src/v8.cc
File src/v8.cc (right):

http://codereview.chromium.org/10807024/diff/2001/src/v8.cc#newcode110
src/v8.cc:110: delete isolate;
Why move this code?

http://codereview.chromium.org/10807024/diff/2001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

http://codereview.chromium.org/10807024/diff/2001/src/x64/builtins-x64.cc#new...
src/x64/builtins-x64.cc:81: __ addq(kScratchRegister,
Immediate(Code::kHeaderSize - kHeapObjectTag));
I'd prefer if you use the lea-instruction and FieldOperand here to compute the
jump target (easier to read).

http://codereview.chromium.org/10807024/diff/2001/test/mjsunit/array-bounds-c...
File test/mjsunit/array-bounds-check-removal.js (right):

http://codereview.chromium.org/10807024/diff/2001/test/mjsunit/array-bounds-c...
test/mjsunit/array-bounds-check-removal.js:28: // Flags: --allow-natives-syntax
--expose-gc --noparallel-recompilation
Can't we force non-threaded compilation when we use %OptimizeFunctionOnNextCall?
Or have it stall execution until function has been installed?

[Yang, 2012-07-19 14:26:31]

A few more comments. LGTM if all comments are addressed.

http://codereview.chromium.org/10807024/diff/2001/src/isolate.h
File src/isolate.h (right):

http://codereview.chromium.org/10807024/diff/2001/src/isolate.h#newcode56
src/isolate.h:56: class CompilerProxy;
Left over? I can't find any CompilerProxy class.

http://codereview.chromium.org/10807024/diff/2001/src/isolate.h#newcode1294
src/isolate.h:1294: CompilerProxy* compiler_proxy_;
ditto.

http://codereview.chromium.org/10807024/diff/2001/src/smart-pointers.h
File src/smart-pointers.h (right):

http://codereview.chromium.org/10807024/diff/2001/src/smart-pointers.h#newcod...
src/smart-pointers.h:139: #endif  // V8_SMART_ARRAY_POINTERS_H_
You probably mean V8_SMART_POINTERS_H_ in this comment.

[sanjoy, 2012-07-19 15:06:09]

* Edited NoHandleAllocation and AssertNoAllocation to be null operations when
activated on the compiler thread.

 * Calling CompilationInfo::AbortOptimization from
OptimizingCompiler::OptimizeGraph from the compiler thread (rightly) crashes v8
in debug mode.  Fixed this.

http://codereview.chromium.org/10807024/diff/2001/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/10807024/diff/2001/src/compiler.cc#newcode875
src/compiler.cc:875: // If crankshaft succeeded, install the optimized code else
install
On 2012/07/19 12:47:30, Yang wrote:
> "If crankshaft succeeded, install the optimized code. Otherwise install the
> unoptimized code."

Done.

http://codereview.chromium.org/10807024/diff/2001/src/execution.cc
File src/execution.cc (right):

http://codereview.chromium.org/10807024/diff/2001/src/execution.cc#newcode966
src/execution.cc:966: 
On 2012/07/19 12:47:30, Yang wrote:
> Stray edit?

Done.

http://codereview.chromium.org/10807024/diff/2001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/10807024/diff/2001/src/heap.cc#newcode6252
src/heap.cc:6252: if (FLAG_parallel_recompilation) delete relocation_mutex_;
On 2012/07/19 12:47:30, Yang wrote:
> I think you can omit checking for the flag since deleting NULL is safe.

Done.

http://codereview.chromium.org/10807024/diff/2001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (left):

http://codereview.chromium.org/10807024/diff/2001/src/hydrogen-instructions.h...
src/hydrogen-instructions.h:2292: ASSERT(!HEAP->IsAllocationAllowed());
On 2012/07/19 12:47:30, Yang wrote:
> It would be great if we could distinguish between the threads to still check
> that the optimizing compile thread does not allocate. Since we never allocate
> anything in the optimizing compile thread, why not have every place that
checks
> for allocation_allowed_ to also check that the thread is not the optimizing
> compile thread?

We can't leave in the checks as-is since the JS thread probably has allowed
allocation on the heap by now.  But they can be made conditional on which thread
we're executing in.

http://codereview.chromium.org/10807024/diff/2001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

http://codereview.chromium.org/10807024/diff/2001/src/ia32/builtins-ia32.cc#n...
src/ia32/builtins-ia32.cc:80: __ add(eax, Immediate(Code::kHeaderSize -
kHeapObjectTag));
On 2012/07/19 12:47:30, Yang wrote:
> Use the lea instruction and FieldOperand would be easier to read.

Done.

http://codereview.chromium.org/10807024/diff/2001/src/isolate.h
File src/isolate.h (left):

http://codereview.chromium.org/10807024/diff/2001/src/isolate.h#oldcode764
src/isolate.h:764: 
On 2012/07/19 12:47:30, Yang wrote:
> Stray edit?

Done.

http://codereview.chromium.org/10807024/diff/2001/src/isolate.h
File src/isolate.h (right):

http://codereview.chromium.org/10807024/diff/2001/src/isolate.h#newcode56
src/isolate.h:56: class CompilerProxy;
On 2012/07/19 14:26:31, Yang wrote:
> Left over? I can't find any CompilerProxy class.

Yeah, fixed.

http://codereview.chromium.org/10807024/diff/2001/src/isolate.h#newcode1294
src/isolate.h:1294: CompilerProxy* compiler_proxy_;
On 2012/07/19 14:26:31, Yang wrote:
> ditto.

Done.

http://codereview.chromium.org/10807024/diff/2001/src/optimizing-compiler-thr...
File src/optimizing-compiler-thread.cc (right):

http://codereview.chromium.org/10807024/diff/2001/src/optimizing-compiler-thr...
src/optimizing-compiler-thread.cc:81: OptimizingCompiler* compiler = NULL;
On 2012/07/19 12:47:30, Yang wrote:
> Maybe OptimizingCompileTask would be a better name than OptimizingCompiler.

I'm not sure about this -- MakeCrankshaftCode uses OptimizingCompiler just like
a compiler, for instance.  It really does have all the state and the code needed
to fully optimize a function.

http://codereview.chromium.org/10807024/diff/2001/src/smart-pointers.h
File src/smart-pointers.h (right):

http://codereview.chromium.org/10807024/diff/2001/src/smart-pointers.h#newcod...
src/smart-pointers.h:139: #endif  // V8_SMART_ARRAY_POINTERS_H_
On 2012/07/19 14:26:31, Yang wrote:
> You probably mean V8_SMART_POINTERS_H_ in this comment.

Done.

http://codereview.chromium.org/10807024/diff/2001/src/v8.cc
File src/v8.cc (right):

http://codereview.chromium.org/10807024/diff/2001/src/v8.cc#newcode110
src/v8.cc:110: delete isolate;
On 2012/07/19 12:47:30, Yang wrote:
> Why move this code?

Added comment.

http://codereview.chromium.org/10807024/diff/2001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

http://codereview.chromium.org/10807024/diff/2001/src/x64/builtins-x64.cc#new...
src/x64/builtins-x64.cc:81: __ addq(kScratchRegister,
Immediate(Code::kHeaderSize - kHeapObjectTag));
On 2012/07/19 12:47:30, Yang wrote:
> I'd prefer if you use the lea-instruction and FieldOperand here to compute the
> jump target (easier to read).

Done.

http://codereview.chromium.org/10807024/diff/2001/test/mjsunit/array-bounds-c...
File test/mjsunit/array-bounds-check-removal.js (right):

http://codereview.chromium.org/10807024/diff/2001/test/mjsunit/array-bounds-c...
test/mjsunit/array-bounds-check-removal.js:28: // Flags: --allow-natives-syntax
--expose-gc --noparallel-recompilation
On 2012/07/19 12:47:30, Yang wrote:
> Can't we force non-threaded compilation when we use
%OptimizeFunctionOnNextCall?
> Or have it stall execution until function has been installed?

I removed this by accident -- %OptimizeFunctionOnNextCall has correct semantics
even when compiling things parallely.

[Yang, 2012-07-19 15:56:01]

LGTM with comment.

http://codereview.chromium.org/10807024/diff/1102/src/compiler.h
File src/compiler.h (right):

http://codereview.chromium.org/10807024/diff/1102/src/compiler.h#newcode382
src/compiler.h:382: bool abort_optimization_;
imo checking for BAILED_OUT status after the parallelizable phase is sufficient
to determine whether to DoPendingAbort. Also please add assertions to
AbortOptimization and DeferAbortOptimization that we are in the expected thread
(if FLAG_parallel_recompilation).

[Yang, 2012-07-19 16:11:56]

One more comment.

http://codereview.chromium.org/10807024/diff/1102/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/10807024/diff/1102/src/compiler.cc#newcode824
src/compiler.cc:824: ASSERT(!closure->IsInRecompileQueue());
As discussed, this should probably be a real check (return if already being
optimized) instead of an assertion.

[sanjoy, 2012-07-19 16:25:36]

Other than addressing comments, changed IsCompilerThread() to return false if
recompilation is false.  This simplifies the assertions and is more intuitive --
when FLAG_parallel_recompilation is false, we are never on the compiler thread.

http://codereview.chromium.org/10807024/diff/1102/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/10807024/diff/1102/src/compiler.cc#newcode824
src/compiler.cc:824: ASSERT(!closure->IsInRecompileQueue());
On 2012/07/19 16:11:56, Yang wrote:
> As discussed, this should probably be a real check (return if already being
> optimized) instead of an assertion.

Done.

http://codereview.chromium.org/10807024/diff/1102/src/compiler.h
File src/compiler.h (right):

http://codereview.chromium.org/10807024/diff/1102/src/compiler.h#newcode382
src/compiler.h:382: bool abort_optimization_;
On 2012/07/19 15:56:01, Yang wrote:
> imo checking for BAILED_OUT status after the parallelizable phase is
sufficient
> to determine whether to DoPendingAbort. Also please add assertions to
> AbortOptimization and DeferAbortOptimization that we are in the expected
thread
> (if FLAG_parallel_recompilation).

Done.

[Yang, 2012-07-19 16:37:16]

LGTM if comments addressed.

http://codereview.chromium.org/10807024/diff/5007/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/10807024/diff/5007/src/compiler.cc#newcode357
src/compiler.cc:357: return SetLastStatus(BAILED_OUT);
Do we handle BAILED_OUT when running this in the parallel thread at all?

http://codereview.chromium.org/10807024/diff/5007/src/optimizing-compiler-thr...
File src/optimizing-compiler-thread.cc (right):

http://codereview.chromium.org/10807024/diff/5007/src/optimizing-compiler-thr...
src/optimizing-compiler-thread.cc:62: (void) status;
What does this line do?

http://codereview.chromium.org/10807024/diff/5007/src/optimizing-compiler-thr...
src/optimizing-compiler-thread.cc:101: return false;
Put this on the previous line or in {}.