On 2013/11/13 17:18:09, danno wrote:
Going in the right direction! Feedback is mostly nits...
https://codereview.chromium.org/70013002/diff/40001/src/flag-definitions.h
File src/flag-definitions.h (right):
https://codereview.chromium.org/70013002/diff/40001/src/flag-definitions.h#newcode790
src/flag-definitions.h:790: DEFINE_bool(perf_basic_prof, false, "Enable
perf
linux profiler (basic support).")
nit: 80 col (tools/presubmit should have pointed this out, BTW)
https://codereview.chromium.org/70013002/diff/40001/src/flag-definitions.h#newcode791
src/flag-definitions.h:791: DEFINE_bool(perf_jit_prof, false, "Enable perf
linux
profiler "
How about call this one perf_annotate_support?
https://codereview.chromium.org/70013002/diff/40001/src/flag-definitions.h#newcode792
src/flag-definitions.h:792: "(experimental annotate support).")
You probably want to make sure that these flags imply
nocompact_code_space, or
at least in MarkCompactCollector::StartCompaction make sure, just like
with
the
gdbjit, that when these features are on, there is no compaction possible.
https://codereview.chromium.org/70013002/diff/40001/src/log.cc
File src/log.cc (right):
https://codereview.chromium.org/70013002/diff/40001/src/log.cc#newcode252
src/log.cc:252: explicit PerfBasicLogger();
Don't need explicit here, only for single argument constructors.
https://codereview.chromium.org/70013002/diff/40001/src/log.cc#newcode285
src/log.cc:285: perf_dump_name,
nit: 4 char indent
https://codereview.chromium.org/70013002/diff/40001/src/log.cc#newcode307
src/log.cc:307: (uint64_t)code->instruction_start(),
nit: 4 char indent (2 is for nested scopes only)
https://codereview.chromium.org/70013002/diff/40001/src/log.cc#newcode316
src/log.cc:316: explicit PerfJitLogger();
Don't need explicit here, only for single argument constructor.
https://codereview.chromium.org/70013002/diff/40001/src/log.cc#newcode404
src/log.cc:404: perf_dump_name,
Indent 4 characters on line continuations
https://codereview.chromium.org/70013002/diff/40001/src/log.cc#newcode422
src/log.cc:422: SharedFunctionInfo*,
nit: indentation one char too many on this line and below.
Addressed everything in patch set 3, except the change of the flag name. The
--perf-jit-prof flag does not support just 'perf annotate', it also supports
'perf report'. Once the perf tool changes make it to the mainline kernel, we
should only keep --perf-jit-prof.
I should also say that with release build of v8, I see that there are still
some
addresses that are not accounted for. I will try to chase them down. Today,
I
have also seen 'perf report' crashing occasionally on start, so there is
still
some something funny going on...
https://codereview.chromium.org/70013002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
