Thanks for the comments! Please take another look.

-- Vitaly


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)];
On 2010/10/17 19:51:12, Michail Naganov wrote:
ScopedVector<char>?

Done.

http://codereview.chromium.org/3831002/diff/3001/4004#newcode158
src/log-utils.cc:158: memcpy(code_name, name, name_len);
On 2010/10/17 19:51:12, Michail Naganov wrote:
OS::StrNCpy?

I need the length anyway, so memcpy is easier.

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_;
On 2010/10/17 19:51:12, Michail Naganov wrote:
Please add a comment for this variable.

Done.

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_));
On 2010/10/17 19:51:12, Michail Naganov wrote:
Should we put ftell into OS interface, like FOpen?

Probably not worth it while the standard C functions do the job.

http://codereview.chromium.org/3831002/diff/3001/4006#newcode1362
src/log.cc:1362: fwrite(code->instruction_start(), 1,
code->instruction_size(),
On 2010/10/17 19:51:12, Michail Naganov wrote:
and FWrite

Same here.

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
On 2010/10/17 19:51:12, Michail Naganov wrote:
My usual solution to eliminate such dependencies is to include all
needed
constants into the log itself and read them by the analyzer.

I added code-info log entry that is used to pass the architecture and
code header size.

http://codereview.chromium.org/3831002/diff/3001/4016#newcode218
tools/ll_prof.py:218: command = "%s %s -D -b binary %s %s" % (
On 2010/10/17 19:51:12, Michail Naganov wrote:
You may consider using dd for getting a range of bytes from a file.

Good idea. Done.

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]+)")
On 2010/10/17 19:51:12, Michail Naganov wrote:
Why a-z?

Fixed (in other places too).

http://codereview.chromium.org/3831002/diff/3001/4016#newcode511
tools/ll_prof.py:511: TRACE_HEADER_DESC = Descriptor([
On 2010/10/17 19:51:12, Michail Naganov wrote:
I think it's a good idea to add a link to perf (?) documentation here.

I doesn't seem to have an up-to-date site, but the documentation in the
kernel repo is pretty good. Added a link to it.

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

Right, the kernel sends perf an MMAP event each time the process mmap's
something with PROT_EXEC. This also covers the binary itself and the
even the kernel modules (!).

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

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

Reply via email to