LGTM if comments are addressed (one is already obsolete in patch set 11,
but I
was too lazy to remove it).
https://codereview.chromium.org/11412125/diff/3017/src/compiler.cc
File src/compiler.cc (right):
https://codereview.chromium.org/11412125/diff/3017/src/compiler.cc#newcode858
src/compiler.cc:858: HistogramTimerScope
timer(isolate->counters()->recompile_synchronous());
Adding the run time of a method named RecompileParallel() to a counter
named recompile_synchronous is surprising. I assume it's on purpose,
though, because this is the non-backgrounded "prologue", right? Please
add a comment to that effect.
https://codereview.chromium.org/11412125/diff/3017/src/log.h
File src/log.h (right):
https://codereview.chromium.org/11412125/diff/3017/src/log.h#newcode292
src/log.h:292: Logger* logger_;
"private:"?
https://codereview.chromium.org/11412125/diff/3017/tools/plot-timer-events.js
File tools/plot-timer-events.js (right):
https://codereview.chromium.org/11412125/diff/3017/tools/plot-timer-events.js#newcode31
tools/plot-timer-events.js:31: 'V8.ScriptRun' :
nit: free-floating colons look weird. I'd remove the spaces before them.
https://codereview.chromium.org/11412125/diff/3017/tools/plot-timer-events.js#newcode76
tools/plot-timer-events.js:76:
nit: two empty lines between top-level statements.
https://codereview.chromium.org/11412125/diff/3017/tools/plot-timer-events.js#newcode148
tools/plot-timer-events.js:148: if ((next_range.end > merge_end)) { //
Extend range end.
nit: duplicate parentheses
https://codereview.chromium.org/11412125/diff/3017/tools/plot-timer-events.js#newcode208
tools/plot-timer-events.js:208: include_start = exclude_start;
s/exclude_start/exclude_end/, I think
https://codereview.chromium.org/11412125/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
