Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1045753002:
CpuProfiler: public API for deopt info in cpu profiler. (Closed)
DescriptionCpuProfiler: public API for deopt info in cpu profiler.
BUG=chromium:452067
LOG=n
Committed: https://crrev.com/baf927ff5115ec62a6dad684b9232ed9d3960e3a
Cr-Commit-Position: refs/heads/master@{#27626}
Committed: https://crrev.com/eb95406e2bc46e14efac17a771a21989a59a6ec9
Cr-Commit-Position: refs/heads/master@{#27674}
Patch Set 1 : size_t #
Total comments: 4
Patch Set 2 : comments addressed #
Total comments: 8
Patch Set 3 : comments addressed #
Total comments: 12
Patch Set 4 : comments addressed #Patch Set 5 : comments addressed #Patch Set 6 : rebased #Patch Set 7 : fix for win32 no_snapshot version #Patch Set 8 : second try #Patch Set 9 : unnecessary typedef was removed #
Messages
Total messages: 41 (17 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
loislo@chromium.org changed reviewers: + alph@chromium.org, yurys@chromium.org
PTAL
lgtm https://codereview.chromium.org/1045753002/diff/50001/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/1045753002/diff/50001/include/v8-profiler.h#n... include/v8-profiler.h:102: /** Retrieves a child node by index. */ The comment belongs to the second function. https://codereview.chromium.org/1045753002/diff/50001/test/cctest/test-cpu-pr... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/1045753002/diff/50001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:59: static unsigned int offset(const char* src, const char* substring) { could you please add a check that strstr is not null.
comments addressed https://codereview.chromium.org/1045753002/diff/50001/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/1045753002/diff/50001/include/v8-profiler.h#n... include/v8-profiler.h:102: /** Retrieves a child node by index. */ On 2015/03/30 at 12:31:21, alph wrote: > The comment belongs to the second function. done. https://codereview.chromium.org/1045753002/diff/50001/test/cctest/test-cpu-pr... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/1045753002/diff/50001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:59: static unsigned int offset(const char* src, const char* substring) { On 2015/03/30 at 12:31:21, alph wrote: > could you please add a check that strstr is not null. done
still lg https://codereview.chromium.org/1045753002/diff/70001/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/1045753002/diff/70001/include/v8-profiler.h#n... include/v8-profiler.h:35: * then we will get 3 frames. The first for C where deopt happened. ... then there will be 3 frames. https://codereview.chromium.org/1045753002/diff/70001/include/v8-profiler.h#n... include/v8-profiler.h:42: /** Returns the deopt frame and a few inline frames.*/ drop 'a few'
https://codereview.chromium.org/1045753002/diff/70001/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/1045753002/diff/70001/include/v8-profiler.h#n... include/v8-profiler.h:25: unsigned int position; unsigned https://codereview.chromium.org/1045753002/diff/70001/include/v8-profiler.h#n... include/v8-profiler.h:40: unsigned GetFramesCount() const; Why not return a pointer/reference to std::vector<Frame> which would be more concise and efficient. AFAIU there is no taboo on using STL in V8 public API anymore, right? https://codereview.chromium.org/1045753002/diff/70001/include/v8-profiler.h#n... include/v8-profiler.h:43: bool GetCallFrames(Frame* frames, unsigned int length) const; ditto https://codereview.chromium.org/1045753002/diff/70001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1045753002/diff/70001/src/api.cc#newcode7523 src/api.cc:7523: DCHECK_LT(index, node->deopt_infos().size()); Drop this as List already performs such checks. https://codereview.chromium.org/1045753002/diff/70001/src/api.cc#newcode7542 src/api.cc:7542: bool CpuProfileDeoptInfo::GetCallFrames(Frame* frames, Return number of written frames?
jkummerow@chromium.org changed reviewers: + jkummerow@chromium.org
DBC. Please wait with landing any API changes until after the upcoming branch point. (I see that these changes by themselves are backwards compatible, but we need rollback flexibility, and the only way to ensure that no Chrome/Blink features start depending on new V8 versions is by not landing API changes in the first place.) https://codereview.chromium.org/1045753002/diff/70001/test/cctest/test-cpu-pr... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/1045753002/diff/70001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:61: ASSERT(it); You mean DCHECK.
loislo@chromium.org changed reviewers: + svenpanne@chromium.org
PTAL
https://codereview.chromium.org/1045753002/diff/90001/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/1045753002/diff/90001/include/v8-profiler.h#n... include/v8-profiler.h:25: unsigned int position; unsigned int -> unsigned https://codereview.chromium.org/1045753002/diff/90001/src/profile-generator.h File src/profile-generator.h (right): https://codereview.chromium.org/1045753002/diff/90001/src/profile-generator.h... src/profile-generator.h:40: typedef v8::CpuProfileDeoptInfo DeoptInfo; Can we use original type without the alias. It seems to be more clear. https://codereview.chromium.org/1045753002/diff/90001/test/cctest/test-cpu-pr... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/1045753002/diff/90001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:59: static unsigned int offset(const char* src, const char* substring) { unsigned int -> unsigned https://codereview.chromium.org/1045753002/diff/90001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:62: return static_cast<unsigned int>(it - src); unsigned int -> unsigned
https://codereview.chromium.org/1045753002/diff/90001/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/1045753002/diff/90001/include/v8-profiler.h#n... include/v8-profiler.h:25: unsigned int position; On 2015/03/30 20:33:15, yurys wrote: > unsigned int -> unsigned I think that size_t is the right type here, isn't it? It might be the case that we have wrong types internally, but at least we should get things right on the external API. https://codereview.chromium.org/1045753002/diff/90001/include/v8-profiler.h#n... include/v8-profiler.h:27: const char* deopt_reason; Add a comment about ownership here, naked pointer are always a bit vague...
https://codereview.chromium.org/1045753002/diff/90001/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/1045753002/diff/90001/include/v8-profiler.h#n... include/v8-profiler.h:25: unsigned int position; On 2015/03/31 06:27:09, Sven Panne wrote: > On 2015/03/30 20:33:15, yurys wrote: > > unsigned int -> unsigned > > I think that size_t is the right type here, isn't it? It might be the case that > we have wrong types internally, but at least we should get things right on the > external API. Agree, size_t would be even better.
On 2015/03/31 at 06:53:49, yurys wrote: > https://codereview.chromium.org/1045753002/diff/90001/include/v8-profiler.h > File include/v8-profiler.h (right): > > https://codereview.chromium.org/1045753002/diff/90001/include/v8-profiler.h#n... > include/v8-profiler.h:25: unsigned int position; > On 2015/03/31 06:27:09, Sven Panne wrote: > > On 2015/03/30 20:33:15, yurys wrote: > > > unsigned int -> unsigned > > > > I think that size_t is the right type here, isn't it? It might be the case that > > we have wrong types internally, but at least we should get things right on the > > external API. > > Agree, size_t would be even better. done
LGTM, but please wait until after the branch point, as Jakob pointed out.
https://codereview.chromium.org/1045753002/diff/90001/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/1045753002/diff/90001/include/v8-profiler.h#n... include/v8-profiler.h:25: unsigned int position; On 2015/03/31 at 06:53:49, yurys wrote: > On 2015/03/31 06:27:09, Sven Panne wrote: > > On 2015/03/30 20:33:15, yurys wrote: > > > unsigned int -> unsigned > > > > I think that size_t is the right type here, isn't it? It might be the case that > > we have wrong types internally, but at least we should get things right on the > > external API. > > Agree, size_t would be even better. done https://codereview.chromium.org/1045753002/diff/90001/include/v8-profiler.h#n... include/v8-profiler.h:27: const char* deopt_reason; On 2015/03/31 at 06:27:09, Sven Panne wrote: > Add a comment about ownership here, naked pointer are always a bit vague... done https://codereview.chromium.org/1045753002/diff/90001/src/profile-generator.h File src/profile-generator.h (right): https://codereview.chromium.org/1045753002/diff/90001/src/profile-generator.h... src/profile-generator.h:40: typedef v8::CpuProfileDeoptInfo DeoptInfo; On 2015/03/30 at 20:33:16, yurys wrote: > Can we use original type without the alias. It seems to be more clear. done https://codereview.chromium.org/1045753002/diff/90001/test/cctest/test-cpu-pr... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/1045753002/diff/90001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:59: static unsigned int offset(const char* src, const char* substring) { On 2015/03/30 at 20:33:16, yurys wrote: > unsigned int -> unsigned done https://codereview.chromium.org/1045753002/diff/90001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:62: return static_cast<unsigned int>(it - src); On 2015/03/30 at 20:33:16, yurys wrote: > unsigned int -> unsigned done
The CQ bit was checked by loislo@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org, svenpanne@chromium.org Link to the patchset: https://codereview.chromium.org/1045753002/#ps130001 (title: "comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1045753002/130001
On 2015/03/31 at 08:23:34, commit-bot wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1045753002/130001 I started CQ dry run
The CQ bit was unchecked by commit-bot@chromium.org
This issue passed the CQ dry run.
The CQ bit was checked by loislo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from svenpanne@chromium.org, alph@chromium.org Link to the patchset: https://codereview.chromium.org/1045753002/#ps150001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1045753002/150001
Message was sent while issue was closed.
Committed patchset #6 (id:150001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/baf927ff5115ec62a6dad684b9232ed9d3960e3a Cr-Commit-Position: refs/heads/master@{#27626}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:150001) has been created in https://codereview.chromium.org/1062053004/ by machenbach@chromium.org. The reason for reverting is: [Sheriff] Breaks compile here: http://build.chromium.org/p/client.v8/builders/V8%20Win32%20-%20nosnap%20-%20....
The CQ bit was checked by loislo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from svenpanne@chromium.org, alph@chromium.org Link to the patchset: https://codereview.chromium.org/1045753002/#ps190001 (title: "second try")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1045753002/190001
The CQ bit was unchecked by loislo@chromium.org
The CQ bit was checked by loislo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from svenpanne@chromium.org, alph@chromium.org Link to the patchset: https://codereview.chromium.org/1045753002/#ps210001 (title: "unnecessary typedef was removed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1045753002/210001
Message was sent while issue was closed.
Committed patchset #9 (id:210001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/eb95406e2bc46e14efac17a771a21989a59a6ec9 Cr-Commit-Position: refs/heads/master@{#27674} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
