[v8-dev] Re: Optimize functions on a second thread. (issue 10807024)

[yangguo]

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#oldcode2292
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#newcode80
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-thread.cc
File src/optimizing-compiler-thread.cc (right):

http://codereview.chromium.org/10807024/diff/2001/src/optimizing-compiler-thread.cc#newcode81
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#newcode81
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-check-removal.js
File test/mjsunit/array-bounds-check-removal.js (right):

http://codereview.chromium.org/10807024/diff/2001/test/mjsunit/array-bounds-check-removal.js#newcode28
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?

http://codereview.chromium.org/10807024/

--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev