1322663 - Add Code Coverage endpoints to Debugger Server

Status: NEW

(DevTools :: Debugger, enhancement, P5)

Description

[Jason Laster [:jlast]]

This bug is about adding code coverage support to the debugger server, so that the client can fetch code coverage information with the script API: script.getOffsetsCoverage.

Comment 1

[Jason Laster [:jlast]]

Attachment: cov2.patch

This is a WIP. The two big questions I have are: 

1. when should I enable coverage. Based on prior discussions, I believe we should update the threadClient endpoint so it starts and captures a recording

2. how should we handle naming and signatures. This is something I threw together quickly and we can discuss here.

Comment 2

[Jason Laster [:jlast]]

Nicolas, curious to get your thoughts on the API.

Comment 3

[James Long (:jlongster)]

Comment on attachment 8817588 [details] [diff] [review]
cov2.patch

Review of attachment 8817588 [details] [diff] [review]:
-----------------------------------------------------------------

The main problem is that the recording is on by default. The UI needs to turn recording on explicitly so there isn't a default perf overhead whenever you open the devtools.

Thanks for working on this though, it would be nice to have!

::: devtools/server/actors/script.js
@@ +464,5 @@
>        // Keep the debugger disabled until a client attaches.
>        this._dbg.enabled = this._state != "detached";
> +
> +      // start code coverage
> +      this._dbg.collectCoverageInfo = true;

Yes, as you mentioned in your comment, you don't want to turn it on by default. It should be default off so there is no overhead, and some sort of UX figured out to "record" coverage and only turn it on when recording. You'll need additional RDP methods for that.

::: devtools/server/actors/source.js
@@ +626,5 @@
> +  hitCounts() {
> +    let source = this.source || this.generatedSource;
> +
> +    if (!source) {
> +        return;

Don't return undefined, return the same type as the function expects.

@@ +635,5 @@
> +    let hitcounts = {};
> +
> +    scripts.forEach(script => {
> +       let coverage = script.getOffsetsCoverage();
> +       if (!coverage) return;

Avoid early returns where possible; here just do `if (coverage)` and wrap the code. Early returns end up making code hard to follow.

@@ +638,5 @@
> +       let coverage = script.getOffsetsCoverage();
> +       if (!coverage) return;
> +
> +       coverage.forEach((data, i) => {
> +         hitcounts[data.lineNumber] = Math.max(hitcounts[data.lineNumber] || 0, data.count);

Are you sure this is correct? I would assume that the script has the number of times itself has been hit, so you should take the max of all the scripts on the same line, you should add them together.

Comment 4

[James Long (:jlongster)]

> so you should

That should be "so you shouldn't"

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to James Long (:jlongster) from comment #3)
> ::: devtools/server/actors/script.js
> @@ +464,5 @@
> >        // Keep the debugger disabled until a client attaches.
> >        this._dbg.enabled = this._state != "detached";
> > +
> > +      // start code coverage
> > +      this._dbg.collectCoverageInfo = true;
> 
> Yes, as you mentioned in your comment, you don't want to turn it on by
> default. It should be default off so there is no overhead, and some sort of
> UX figured out to "record" coverage and only turn it on when recording.
> You'll need additional RDP methods for that.

The code coverage data is collected by default in Baseline compiled code, but not in Ion, nor the interpreter, unless this Debugger flag is set to "true".

Setting this flag to "true" will slow down the interpreter by ~2x.  Baseline code is already instrumented with code coverage counters for Branch Pruning optimizations.

Ion seems to be lacking instrumentation, and does not seems to be disabled as well.  I will write a test case to ensure that we disable/instrument Ion.

> @@ +638,5 @@
> > +       let coverage = script.getOffsetsCoverage();
> > +       if (!coverage) return;
> > +
> > +       coverage.forEach((data, i) => {
> > +         hitcounts[data.lineNumber] = Math.max(hitcounts[data.lineNumber] || 0, data.count);
> 
> Are you sure this is correct? I would assume that the script has the number
> of times itself has been hit, so you should take the max of all the scripts
> on the same line, you should add them together.

The problem here, is that the code coverage report is made on a bytecode-offset basis and not on a line-offset basis.  Doing a sum, might produce confusing outputs such as:

50    var a = foo();
150   var b = a ? 1 : 0;

The reason being that the "?:" will appear as multiple bytecode instructions which are sharing the same line.

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8817588 [details] [diff] [review]
cov2.patch

Review of attachment 8817588 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/server/actors/source.js
@@ +643,5 @@
> +       });
> +     });
> +
> +     return Object.keys(hitcounts).map(key => ({
> +       line: +key,

Any reason not to use an array indexed by line numbers?