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); On 2012/07/16 12:19:09, danno wrote:
nit: Move this down after the comment so diff is clearer.
Done. http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode268 src/compiler.cc:268: Handle<Context> global_context( On 2012/07/16 12:19:09, danno wrote:
This fits on one line like in the original, no?
No, because I had to replace `info' with `info()'. http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode281 src/compiler.cc:281: On 2012/07/16 12:19:09, danno wrote:
Don't add a line here, please keep original white space as much as
possible to
help make diff clearer.
Done. http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode281 src/compiler.cc:281: On 2012/07/16 12:19:09, danno wrote:
Don't add a line here, please keep original white space as much as
possible to
help make diff clearer.
Done. http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode301 src/compiler.cc:301: if (chunk_ == NULL) { On 2012/07/16 12:19:09, danno wrote:
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?
Done. http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode324 src/compiler.cc:324: void OptimizingCompiler::RecordOptimizationStats() { On 2012/07/16 12:19:09, danno wrote:
Can you please move this back up to where FinishOptimization used to
be? Makes
the diffs much simpler.
Done. http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode352 src/compiler.cc:352: static bool MakeCrankshaftCode(CompilationInfo* info) { On 2012/07/16 12:19:09, danno wrote:
Can you please move this up where MakeCrankshaftCode used to be?
Again, to
minimize the diffs if possible.
Done. 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 On 2012/07/16 12:19:09, danno wrote:
nit: compilation phases maybe you should list what they are?
Done. http://codereview.chromium.org/10700188/diff/2001/src/compiler.h#newcode337 src/compiler.h:337: Status CreateHGraph() { On 2012/07/16 12:19:09, danno wrote:
Just call this CreateGraph()
Done. http://codereview.chromium.org/10700188/diff/2001/src/compiler.h#newcode341 src/compiler.h:341: Status OptimizeHGraph() { On 2012/07/16 12:19:09, danno wrote:
OptimizeGraph
Done. http://codereview.chromium.org/10700188/diff/2001/src/compiler.h#newcode364 src/compiler.h:364: Status InnerCreateHGraph(); On 2012/07/16 12:19:09, danno wrote:
I don't understand why you need these "Inner" methods. Why not just
implement
directly?
I wanted to have a central place to set last_status_. I've now added a SetLastStatus method that also returns the set status so that I can do `return SetLastStatus(FAILED'. http://codereview.chromium.org/10700188/diff/2001/src/compiler.h#newcode385 src/compiler.h:385: compiler_->total_time_taken_ += (OS::Ticks() - start_); On 2012/07/16 12:19:09, danno wrote:
Perhaps divide this up into three counters, one for each phase so that
we can
track the total time taken plus for each phase?
Done. http://codereview.chromium.org/10700188/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
