Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 10 months ago by Yang Modified:
3 years, 10 months ago Reviewers:
jgruber CC:
v8-reviews_googlegroups.com, kozy, fmeawad Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[debugger] implement per-function code coverage.
Collect code coverage from the available invocation counts.
The granularity is at function level, and invocation counts may
be lost to GC.
Coverage::Collect returns a std::vector of Coverage::ScriptData.
Each ScriptData contains a script ID and a std::vector of
Coverage::RangeEntry.
Each RangeEntry consists of a end position and the invocation
count. The start position is implicit from the end position of
the previous RangeEntry, or 0 if it's the first RangeEntry.
R=jgruber@chromium.org
BUG=v8:5808
Review-Url: https://codereview.chromium.org/2689493002
Cr-Commit-Position: refs/heads/master@{#43072}
Committed: https://chromium.googlesource.com/v8/v8/+/058d7ab7f4644b41be3eacf4f9e3f858df89ff4b
Patch Set 1 #Patch Set 2 : fix includes #Patch Set 3 : fix tests #Patch Set 4 : fix one more test #Patch Set 5 : one more fix, and rename #
Total comments: 11
Patch Set 6 : address comments #
Created: 3 years, 10 months ago
Dependent Patchsets: Messages
Total messages: 35 (24 generated)
The CQ bit was checked by yangguo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
The CQ bit was checked by yangguo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
The CQ bit was checked by yangguo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yangguo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please take a look.
Description was changed from ========== [debugger] implement per-function code coverage. Collect code coverage from the available invocation counts. The granularity is at function level, and invocation counts may be lost to GC. R=jgruber@chromium.org BUG=v8:5808 ========== to ========== [debugger] implement per-function code coverage. Collect code coverage from the available invocation counts. The granularity is at function level, and invocation counts may be lost to GC. Coverage::Collect returns a std::vector of Coverage::ScriptData. Each ScriptData contains a script ID and a std::vector of Coverage::RangeEntry. Each RangeEntry consists of a end position and the invocation count. The start position is implicit from the end position of the previous RangeEntry, or 0 if it's the first RangeEntry. R=jgruber@chromium.org BUG=v8:5808 ==========
Description was changed from ========== [debugger] implement per-function code coverage. Collect code coverage from the available invocation counts. The granularity is at function level, and invocation counts may be lost to GC. Coverage::Collect returns a std::vector of Coverage::ScriptData. Each ScriptData contains a script ID and a std::vector of Coverage::RangeEntry. Each RangeEntry consists of a end position and the invocation count. The start position is implicit from the end position of the previous RangeEntry, or 0 if it's the first RangeEntry. R=jgruber@chromium.org BUG=v8:5808 ========== to ========== [debugger] implement per-function code coverage. Collect code coverage from the available invocation counts. The granularity is at function level, and invocation counts may be lost to GC. Coverage::Collect returns a std::vector of Coverage::ScriptData. Each ScriptData contains a script ID and a std::vector of Coverage::RangeEntry. Each RangeEntry consists of a end position and the invocation count. The start position is implicit from the end position of the previous RangeEntry, or 0 if it's the first RangeEntry. R=jgruber@chromium.org BUG=v8:5808 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/ a couple of nits https://codereview.chromium.org/2689493002/diff/80001/src/debug/debug-coverag... File src/debug/debug-coverage.cc (right): https://codereview.chromium.org/2689493002/diff/80001/src/debug/debug-coverag... src/debug/debug-coverage.cc:56: std::vector<Coverage::RangeEntry> Finish() { return std::move(entries_); } To be safe we could add a check to make sure Add is never called after Finish? https://codereview.chromium.org/2689493002/diff/80001/src/debug/debug-coverag... src/debug/debug-coverage.cc:110: // The shared function infos are sorted by start. Is end also taken into account? I'm thinking of a case in which two functions have the same start position: a: {0, 5} b: {0, 7} Can b ever come before a? I suppose not, but maybe we can add a DCHECK. https://codereview.chromium.org/2689493002/diff/80001/src/debug/debug-coverag... src/debug/debug-coverage.cc:111: DCHECK(stack.back().start <= start); Nit: DCHECK_LE here and below. https://codereview.chromium.org/2689493002/diff/80001/src/debug/debug-coverag... src/debug/debug-coverage.cc:131: result.push_back({script->id(), builder.Finish()}); I guess these vector copies are turned into moves by the compiler.. What about using emplace_back to construct in-place? https://codereview.chromium.org/2689493002/diff/80001/src/debug/debug-coverage.h File src/debug/debug-coverage.h (right): https://codereview.chromium.org/2689493002/diff/80001/src/debug/debug-coverag... src/debug/debug-coverage.h:9: #include "src/isolate.h" Nit: We can do a forward declaration of Isolate here instead of the include.
https://codereview.chromium.org/2689493002/diff/80001/src/debug/debug-coverag... File src/debug/debug-coverage.cc (right): https://codereview.chromium.org/2689493002/diff/80001/src/debug/debug-coverag... src/debug/debug-coverage.cc:131: result.push_back({script->id(), builder.Finish()}); On 2017/02/09 15:43:22, jgruber wrote: > I guess these vector copies are turned into moves by the compiler.. What about > using emplace_back to construct in-place? Actually, we probably need a move constructor for ScriptData.
https://codereview.chromium.org/2689493002/diff/80001/src/debug/debug-coverag... File src/debug/debug-coverage.cc (right): https://codereview.chromium.org/2689493002/diff/80001/src/debug/debug-coverag... src/debug/debug-coverage.cc:56: std::vector<Coverage::RangeEntry> Finish() { return std::move(entries_); } On 2017/02/09 15:43:22, jgruber wrote: > To be safe we could add a check to make sure Add is never called after Finish? Apparently creating a temporary vector, swapping the content with entries_, and return the temporary vector, is the way to go. https://codereview.chromium.org/2689493002/diff/80001/src/debug/debug-coverag... src/debug/debug-coverage.cc:110: // The shared function infos are sorted by start. On 2017/02/09 15:43:22, jgruber wrote: > Is end also taken into account? I'm thinking of a case in which two functions > have the same start position: > > a: {0, 5} > b: {0, 7} > > Can b ever come before a? I suppose not, but maybe we can add a DCHECK. actually b can only come before a, since we parse from outer functions into inner functions. I added a DCHECK. Then again I can't think of a way to do this except for the top-level function. https://codereview.chromium.org/2689493002/diff/80001/src/debug/debug-coverag... src/debug/debug-coverage.cc:111: DCHECK(stack.back().start <= start); On 2017/02/09 15:43:22, jgruber wrote: > Nit: DCHECK_LE here and below. Done. https://codereview.chromium.org/2689493002/diff/80001/src/debug/debug-coverag... src/debug/debug-coverage.cc:131: result.push_back({script->id(), builder.Finish()}); On 2017/02/09 15:43:22, jgruber wrote: > I guess these vector copies are turned into moves by the compiler.. What about > using emplace_back to construct in-place? Done. https://codereview.chromium.org/2689493002/diff/80001/src/debug/debug-coverag... src/debug/debug-coverage.cc:131: result.push_back({script->id(), builder.Finish()}); On 2017/02/09 15:53:22, jgruber wrote: > On 2017/02/09 15:43:22, jgruber wrote: > > I guess these vector copies are turned into moves by the compiler.. What about > > using emplace_back to construct in-place? > > Actually, we probably need a move constructor for ScriptData. We are fine with the default move constructor?
The CQ bit was checked by yangguo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jgruber@chromium.org Link to the patchset: https://codereview.chromium.org/2689493002/#ps100001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/09 17:11:01, Yang wrote: > https://codereview.chromium.org/2689493002/diff/80001/src/debug/debug-coverag... > File src/debug/debug-coverage.cc (right): > > https://codereview.chromium.org/2689493002/diff/80001/src/debug/debug-coverag... > src/debug/debug-coverage.cc:56: std::vector<Coverage::RangeEntry> Finish() { > return std::move(entries_); } > On 2017/02/09 15:43:22, jgruber wrote: > > To be safe we could add a check to make sure Add is never called after Finish? > > Apparently creating a temporary vector, swapping the content with entries_, and > return the temporary vector, is the way to go. > > https://codereview.chromium.org/2689493002/diff/80001/src/debug/debug-coverag... > src/debug/debug-coverage.cc:110: // The shared function infos are sorted by > start. > On 2017/02/09 15:43:22, jgruber wrote: > > Is end also taken into account? I'm thinking of a case in which two functions > > have the same start position: > > > > a: {0, 5} > > b: {0, 7} > > > > Can b ever come before a? I suppose not, but maybe we can add a DCHECK. > > actually b can only come before a, since we parse from outer functions into > inner functions. I added a DCHECK. Then again I can't think of a way to do this > except for the top-level function. > > https://codereview.chromium.org/2689493002/diff/80001/src/debug/debug-coverag... > src/debug/debug-coverage.cc:111: DCHECK(stack.back().start <= start); > On 2017/02/09 15:43:22, jgruber wrote: > > Nit: DCHECK_LE here and below. > > Done. > > https://codereview.chromium.org/2689493002/diff/80001/src/debug/debug-coverag... > src/debug/debug-coverage.cc:131: result.push_back({script->id(), > builder.Finish()}); > On 2017/02/09 15:43:22, jgruber wrote: > > I guess these vector copies are turned into moves by the compiler.. What about > > using emplace_back to construct in-place? > > Done. > > https://codereview.chromium.org/2689493002/diff/80001/src/debug/debug-coverag... > src/debug/debug-coverage.cc:131: result.push_back({script->id(), > builder.Finish()}); > On 2017/02/09 15:53:22, jgruber wrote: > > On 2017/02/09 15:43:22, jgruber wrote: > > > I guess these vector copies are turned into moves by the compiler.. What > about > > > using emplace_back to construct in-place? > > > > Actually, we probably need a move constructor for ScriptData. > > We are fine with the default move constructor? Actually not fine with the default move constructor. I had to add constructors for emplace_back.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/20717) v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
The CQ bit was checked by yangguo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/20721) v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
The CQ bit was checked by yangguo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486665451485550,
"parent_rev": "63b980f996c86cfcce724e19e576e95af9e5ba31", "commit_rev":
"058d7ab7f4644b41be3eacf4f9e3f858df89ff4b"}
Message was sent while issue was closed.
Description was changed from ========== [debugger] implement per-function code coverage. Collect code coverage from the available invocation counts. The granularity is at function level, and invocation counts may be lost to GC. Coverage::Collect returns a std::vector of Coverage::ScriptData. Each ScriptData contains a script ID and a std::vector of Coverage::RangeEntry. Each RangeEntry consists of a end position and the invocation count. The start position is implicit from the end position of the previous RangeEntry, or 0 if it's the first RangeEntry. R=jgruber@chromium.org BUG=v8:5808 ========== to ========== [debugger] implement per-function code coverage. Collect code coverage from the available invocation counts. The granularity is at function level, and invocation counts may be lost to GC. Coverage::Collect returns a std::vector of Coverage::ScriptData. Each ScriptData contains a script ID and a std::vector of Coverage::RangeEntry. Each RangeEntry consists of a end position and the invocation count. The start position is implicit from the end position of the previous RangeEntry, or 0 if it's the first RangeEntry. R=jgruber@chromium.org BUG=v8:5808 Review-Url: https://codereview.chromium.org/2689493002 Cr-Commit-Position: refs/heads/master@{#43072} Committed: https://chromium.googlesource.com/v8/v8/+/058d7ab7f4644b41be3eacf4f9e3f858df8... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/v8/v8/+/058d7ab7f4644b41be3eacf4f9e3f858df8... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
