On 2011/02/22 15:11:19, Vitaly wrote:
LGTM.
I'm not a huge fan of the "SFI" abbreviation. Can we at least make it
local to
the profiler internals (where, BTW, we're already
inconsistent: "shared_id"
vs.
"sfi_address") and have "SharedFunctionInfoMoveEvent" in the interface?
Oh, sorry. I forgot to address this comment. Yes, I will make names more
aligned.
Also I didn't find the place where "sample->function" is used to find the
calling function. Does it just work automagically?
http://codereview.chromium.org/6551011/diff/1/src/compiler.h
File src/compiler.h (right):
http://codereview.chromium.org/6551011/diff/1/src/compiler.h#newcode269
src/compiler.h:269: Handle<SharedFunctionInfo> shared);
Can we use info->shared_info() instead of passing another shared function
info?
If not, please document here why.
http://codereview.chromium.org/6551011/diff/1/src/cpu-profiler.cc
File src/cpu-profiler.cc (right):
http://codereview.chromium.org/6551011/diff/1/src/cpu-profiler.cc#newcode358
src/cpu-profiler.cc:358: SharedFunctionInfo *shared,
nit: Move *.
http://codereview.chromium.org/6551011/diff/1/src/cpu-profiler.cc#newcode373
src/cpu-profiler.cc:373: SharedFunctionInfo *shared,
Same nit.
http://codereview.chromium.org/6551011/diff/1/src/handles.cc
File src/handles.cc (right):
http://codereview.chromium.org/6551011/diff/1/src/handles.cc#newcode869
src/handles.cc:869: bool result = CompileLazyHelper(&info,
KEEP_EXCEPTION);
Temporary "result" variable is now unnecessary.
http://codereview.chromium.org/6551011/diff/1/src/log.cc
File src/log.cc (right):
http://codereview.chromium.org/6551011/diff/1/src/log.cc#newcode164
src/log.cc:164: sample->function = Memory::Address_at(sample->sp);
We should rename the "function" field in TickSample. How
about "tos_value"?
http://codereview.chromium.org/6551011/diff/1/src/log.cc#newcode762
src/log.cc:762: if (code == Builtins::builtin(Builtins::LazyCompile))
return;
Can we make the caller do this check for us? (I think it should only
matter
for
calls made from compiler.cc.)
http://codereview.chromium.org/6551011/diff/1/src/log.cc#newcode785
src/log.cc:785: SharedFunctionInfo *shared,
Same nit.
http://codereview.chromium.org/6551011/diff/1/src/log.h
File src/log.h (right):
http://codereview.chromium.org/6551011/diff/1/src/log.h#newcode210
src/log.h:210: SharedFunctionInfo *shared,
nit: Move *.
http://codereview.chromium.org/6551011/diff/1/src/log.h#newcode214
src/log.h:214: SharedFunctionInfo *shared,
Same nit.
http://codereview.chromium.org/6551011/diff/1/src/profile-generator.h
File src/profile-generator.h (right):
http://codereview.chromium.org/6551011/diff/1/src/profile-generator.h#newcode273
src/profile-generator.h:273: static const CodeEntry* kSfiCodeEntry;
Add one more "const" to make it really constant.
http://codereview.chromium.org/6551011/diff/1/tools/ll_prof.py
File tools/ll_prof.py (right):
http://codereview.chromium.org/6551011/diff/1/tools/ll_prof.py#newcode403
tools/ll_prof.py:403: if match.group(6):
Could you please assign these groups (1, 4, 6) to locals (like it's done
for
"start_address") to make this code readable?
http://codereview.chromium.org/6551011/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
