http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc File src/compiler.cc (right):
http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode230 src/compiler.cc:230: Timer t(this); nit: Move this down after the comment so diff is clearer. http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode268 src/compiler.cc:268: Handle<Context> global_context( This fits on one line like in the original, no? http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode281 src/compiler.cc:281: Don't add a line here, please keep original white space as much as possible to help make diff clearer. http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode297 src/compiler.cc:297: return BAILED_OUT; How about AbortCrankshaft just returns a OptimizingCompiler::Status so you can write return AbortCrankshaft(); http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode301 src/compiler.cc:301: if (chunk_ == NULL) { Why will chunk ever be null? It seems like the only path is in the else above, why not put the test there and always return SUCCEEDED at the end of this funciton? http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode324 src/compiler.cc:324: void OptimizingCompiler::RecordOptimizationStats() { Can you please move this back up to where FinishOptimization used to be? Makes the diffs much simpler. http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode352 src/compiler.cc:352: static bool MakeCrankshaftCode(CompilationInfo* info) { Can you please move this up where MakeCrankshaftCode used to be? Again, to minimize the diffs if possible. http://codereview.chromium.org/10700188/diff/2001/src/compiler.h File src/compiler.h (right): http://codereview.chromium.org/10700188/diff/2001/src/compiler.h#newcode319 src/compiler.h:319: // A helper class that calls the three phases in Crankshaft and keeps nit: compilation phases maybe you should list what they are? http://codereview.chromium.org/10700188/diff/2001/src/compiler.h#newcode337 src/compiler.h:337: Status CreateHGraph() { Just call this CreateGraph() http://codereview.chromium.org/10700188/diff/2001/src/compiler.h#newcode341 src/compiler.h:341: Status OptimizeHGraph() { OptimizeGraph http://codereview.chromium.org/10700188/diff/2001/src/compiler.h#newcode364 src/compiler.h:364: Status InnerCreateHGraph(); I don't understand why you need these "Inner" methods. Why not just implement directly? http://codereview.chromium.org/10700188/diff/2001/src/compiler.h#newcode385 src/compiler.h:385: compiler_->total_time_taken_ += (OS::Ticks() - start_); Perhaps divide this up into three counters, one for each phase so that we can track the total time taken plus for each phase? http://codereview.chromium.org/10700188/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
