I'm fine with the change once the couple of issues I pointed out in the
code are
addressed.
Also it is unclear to me whether you are going to add column information to
the
hit count map or you expect it to be done by someone else, please clarify.
https://codereview.chromium.org/424973004/diff/200001/src/cpu-profiler.cc
File src/cpu-profiler.cc (right):
https://codereview.chromium.org/424973004/diff/200001/src/cpu-profiler.cc#newcode261
src/cpu-profiler.cc:261: if (shared->script()->IsScript()) {
It used to be a DCHECK(Script::cast(shared->script())), why we now check
if it is a Script?
https://codereview.chromium.org/424973004/diff/200001/src/cpu-profiler.cc#newcode287
src/cpu-profiler.cc:287: rec->entry->set_bailout_reason(
nit: no need to move these two lines
https://codereview.chromium.org/424973004/diff/200001/src/profile-generator.cc
File src/profile-generator.cc (right):
https://codereview.chromium.org/424973004/diff/200001/src/profile-generator.cc#newcode652
src/profile-generator.cc:652: src_line =
pc_entry->GetSourceLine(pc_offset);
nit: move assignment to src_line after the "if" below.
https://codereview.chromium.org/424973004/diff/200001/src/profile-generator.cc#newcode688
src/profile-generator.cc:688: if (src_line_not_found && *entry) {
With this approach we may end up with top entry being pc_entry while
src_line will be calculated for another entry which is wrong. We should
also check here that current entry is the first one non-null in the
path.
https://codereview.chromium.org/424973004/diff/200001/src/profile-generator.h
File src/profile-generator.h (right):
https://codereview.chromium.org/424973004/diff/200001/src/profile-generator.h#newcode64
src/profile-generator.h:64: return
v8::CpuProfileNode::kNoLineNumberInfo;
It would be more accurate to return last line in this case. Otherwise we
will attribute current sample to the last line only in case of perfect
pc_offset match.
https://codereview.chromium.org/424973004/diff/200001/test/cctest/test-cpu-profiler.cc
File test/cctest/test-cpu-profiler.cc (right):
https://codereview.chromium.org/424973004/diff/200001/test/cctest/test-cpu-profiler.cc#newcode1099
test/cctest/test-cpu-profiler.cc:1099: if
(func->code()->is_optimized_code()) {
You can use "%NeverOptimizeFunction(func)" or
%OptimizeFunctionOnNextCall to make this more deterministic.
https://codereview.chromium.org/424973004/diff/200001/test/cctest/test-cpu-profiler.cc#newcode1138
test/cctest/test-cpu-profiler.cc:1138: CHECK_EQ(false,
line_info->Empty());
nit: CHECK(!line_info->Empty())
https://codereview.chromium.org/424973004/
--
--
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/d/optout.
