Excellent job, Vitaly!

Having this code in place, we can drop OProfile support (as it's only for linux
too, and requires significant efforts to be set up properly).


http://codereview.chromium.org/3831002/diff/3001/4004
File src/log-utils.cc (right):

http://codereview.chromium.org/3831002/diff/3001/4004#newcode157
src/log-utils.cc:157: char* code_name = new char[name_len +
sizeof(kCodeLogExt)];
ScopedVector<char>?

http://codereview.chromium.org/3831002/diff/3001/4004#newcode158
src/log-utils.cc:158: memcpy(code_name, name, name_len);
OS::StrNCpy?

http://codereview.chromium.org/3831002/diff/3001/4005
File src/log-utils.h (right):

http://codereview.chromium.org/3831002/diff/3001/4005#newcode154
src/log-utils.h:154: static FILE* output_code_handle_;
Please add a comment for this variable.

http://codereview.chromium.org/3831002/diff/3001/4006
File src/log.cc (right):

http://codereview.chromium.org/3831002/diff/3001/4006#newcode1361
src/log.cc:1361: int pos =
static_cast<int>(ftell(Log::output_code_handle_));
Should we put ftell into OS interface, like FOpen?

http://codereview.chromium.org/3831002/diff/3001/4006#newcode1362
src/log.cc:1362: fwrite(code->instruction_start(), 1,
code->instruction_size(),
and FWrite

http://codereview.chromium.org/3831002/diff/3001/4016
File tools/ll_prof.py (right):

http://codereview.chromium.org/3831002/diff/3001/4016#newcode72
tools/ll_prof.py:72: JS_CODE_HEADER_SIZE = 32
My usual solution to eliminate such dependencies is to include all
needed constants into the log itself and read them by the analyzer.

http://codereview.chromium.org/3831002/diff/3001/4016#newcode218
tools/ll_prof.py:218: command = "%s %s -D -b binary %s %s" % (
You may consider using dd for getting a range of bytes from a file.

http://codereview.chromium.org/3831002/diff/3001/4016#newcode358
tools/ll_prof.py:358: r"code-move,(0x[a-z0-9]+),(0x[a-f0-9]+)")
Why a-z?

http://codereview.chromium.org/3831002/diff/3001/4016#newcode511
tools/ll_prof.py:511: TRACE_HEADER_DESC = Descriptor([
I think it's a good idea to add a link to perf (?) documentation here.

http://codereview.chromium.org/3831002/diff/3001/4016#newcode894
tools/ll_prof.py:894: mmap_info = trace_reader.ReadMmap(header, offset)
So, pprof emits its own modules map, so we don't need to
LogSharedLibrariesAddresses, right?

http://codereview.chromium.org/3831002/show

--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev

Reply via email to