[debugger] implement per-function code coverage.

[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

[Yang, 2017-02-09 09:23:27]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 09:23:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 09:40:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 09:40:28]

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...)

[Yang, 2017-02-09 10:16:30]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 10:16:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 10:33:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 10:33:59]

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...)

[Yang, 2017-02-09 11:29:20]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 11:29:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yang, 2017-02-09 11:31:23]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 11:31:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yang, 2017-02-09 11:35:52]

Please take a look.

[Yang, 2017-02-09 11:38:47]

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
==========

[Yang, 2017-02-09 11:39:06]

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
==========

[commit-bot: I haz the power, 2017-02-09 11:56:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 11:56:15]

Dry run: This issue passed the CQ dry run.

[jgruber, 2017-02-09 15:43:23]

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.

[jgruber, 2017-02-09 15:53:22]

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.

[Yang, 2017-02-09 17:11:01]

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?

[Yang, 2017-02-09 17:11:07]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2017-02-09 17:11:08]

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")

[commit-bot: I haz the power, 2017-02-09 17:11:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yang, 2017-02-09 17:11:31]

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.

[commit-bot: I haz the power, 2017-02-09 17:39:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 17:39:06]

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...)

[Yang, 2017-02-09 17:44:31]

The CQ bit was checked by yangguo@chromium.org

[commit-bot: I haz the power, 2017-02-09 17:44:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 18:02:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 18:02:33]

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...)

[Yang, 2017-02-09 18:37:31]

The CQ bit was checked by yangguo@chromium.org

[commit-bot: I haz the power, 2017-02-09 18:37:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 19:00:50]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486665451485550,
"parent_rev": "63b980f996c86cfcce724e19e576e95af9e5ba31", "commit_rev":
"058d7ab7f4644b41be3eacf4f9e3f858df89ff4b"}

[commit-bot: I haz the power, 2017-02-09 19:00:54]

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...
==========

[commit-bot: I haz the power, 2017-02-09 19:00:56]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/v8/v8/+/058d7ab7f4644b41be3eacf4f9e3f858df8...