[debugger] add precise mode for code coverage.

[debugger] add precise mode for code coverage.

Collecting precise invocation counts need to be explicitly
enabled. Once enabled, we disable optimization (optimized
code does not increment invocation count, and may inline
callees), and make sure feedback vectors interesting for
code coverage is not garbage-collected.

R=hpayer@chromium.org, jgruber@chromium.org
BUG=v8:5808
Review-Url: https://codereview.chromium.org/2686063002
Cr-Commit-Position: refs/heads/master@{#43082}
Committed: https://chromium.googlesource.com/v8/v8/+/8422e25bb26449e0cf9831adafec48ee4e592278

[Yang, 2017-02-09 11:46:55]

Description was changed from

==========
[debugger] add precise mode for code coverage.

To obtain precise invocation counts, we disable optimization
(optimized code does not increment invocation count, and may
inline callees), and make sure feedback vectors interesting
for code coverage is not garbage-collected.

R=hpayer@chromium.org, jgruber@chromium.org
BUG=v8:5808
==========

to

==========
[debugger] add precise mode for code coverage.

Collecting precise invocation counts need to be explicitly
enabled. Once enabled, we disable optimization (optimized
code does not increment invocation count, and may inline
callees), and make sure feedback vectors interesting for
code coverage is not garbage-collected.

R=hpayer@chromium.org, jgruber@chromium.org
BUG=v8:5808
==========

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

Please take a look.

[jgruber, 2017-02-09 16:08:48]

lgtm with comments

https://codereview.chromium.org/2686063002/diff/1/src/debug/debug-coverage.cc
File src/debug/debug-coverage.cc (right):

https://codereview.chromium.org/2686063002/diff/1/src/debug/debug-coverage.cc...
src/debug/debug-coverage.cc:169:
vectors.push_back(Handle<FeedbackVector>(vector, isolate));
Nit: emplace_back

https://codereview.chromium.org/2686063002/diff/1/src/debug/debug-coverage.cc...
src/debug/debug-coverage.cc:174:
Handle<ArrayList>::cast(isolate->factory()->empty_fixed_array());
We could preallocate a fixed array of the right size here to avoid n
reallocations.

https://codereview.chromium.org/2686063002/diff/1/src/debug/debug-coverage.h
File src/debug/debug-coverage.h (right):

https://codereview.chromium.org/2686063002/diff/1/src/debug/debug-coverage.h#...
src/debug/debug-coverage.h:9: #include "src/allocation.h"
This does not seem to be needed here.

https://codereview.chromium.org/2686063002/diff/1/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2686063002/diff/1/src/isolate.cc#newcode2915
src/isolate.cc:2915: bool Isolate::IsCodeCoverageEnabled() {
Can we mark this const and keep constness above for use_crankshaft? 
And if it's performance-sensitive, perhaps move to the header for inlining.

https://codereview.chromium.org/2686063002/diff/1/src/isolate.cc#newcode2916
src/isolate.cc:2916: return heap()->code_coverage_list()->IsFixedArray();
IsArrayList may be better here and elsewhere for clarity.

https://codereview.chromium.org/2686063002/diff/1/test/mjsunit/code-coverage-...
File test/mjsunit/code-coverage-precise.js (right):

https://codereview.chromium.org/2686063002/diff/1/test/mjsunit/code-coverage-...
test/mjsunit/code-coverage-precise.js:71: %DebugTogglePreciseCoverage(false);
Could you add the same tests as above after disabling precise coverage to
document the difference?

[Yang, 2017-02-09 17:42:15]

https://codereview.chromium.org/2686063002/diff/1/src/debug/debug-coverage.cc
File src/debug/debug-coverage.cc (right):

https://codereview.chromium.org/2686063002/diff/1/src/debug/debug-coverage.cc...
src/debug/debug-coverage.cc:169:
vectors.push_back(Handle<FeedbackVector>(vector, isolate));
On 2017/02/09 16:08:47, jgruber wrote:
> Nit: emplace_back

Done.

https://codereview.chromium.org/2686063002/diff/1/src/debug/debug-coverage.cc...
src/debug/debug-coverage.cc:174:
Handle<ArrayList>::cast(isolate->factory()->empty_fixed_array());
On 2017/02/09 16:08:48, jgruber wrote:
> We could preallocate a fixed array of the right size here to avoid n
> reallocations.

Done.

https://codereview.chromium.org/2686063002/diff/1/src/debug/debug-coverage.h
File src/debug/debug-coverage.h (right):

https://codereview.chromium.org/2686063002/diff/1/src/debug/debug-coverage.h#...
src/debug/debug-coverage.h:9: #include "src/allocation.h"
On 2017/02/09 16:08:48, jgruber wrote:
> This does not seem to be needed here.

This is for the AllStatic base class.

https://codereview.chromium.org/2686063002/diff/1/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2686063002/diff/1/src/isolate.cc#newcode2915
src/isolate.cc:2915: bool Isolate::IsCodeCoverageEnabled() {
On 2017/02/09 16:08:48, jgruber wrote:
> Can we mark this const and keep constness above for use_crankshaft? 
> And if it's performance-sensitive, perhaps move to the header for inlining.

I don't think this is performance sensitive. And unfortunately accessing the
heap is not marked as const. If I did that for the root list accessors, I'll
open a whole new can of worms that I'd rather not.

https://codereview.chromium.org/2686063002/diff/1/src/isolate.cc#newcode2916
src/isolate.cc:2916: return heap()->code_coverage_list()->IsFixedArray();
On 2017/02/09 16:08:48, jgruber wrote:
> IsArrayList may be better here and elsewhere for clarity.

Done.

https://codereview.chromium.org/2686063002/diff/1/test/mjsunit/code-coverage-...
File test/mjsunit/code-coverage-precise.js (right):

https://codereview.chromium.org/2686063002/diff/1/test/mjsunit/code-coverage-...
test/mjsunit/code-coverage-precise.js:71: %DebugTogglePreciseCoverage(false);
On 2017/02/09 16:08:48, jgruber wrote:
> Could you add the same tests as above after disabling precise coverage to
> document the difference?

Done.

[Hannes Payer (out of office), 2017-02-09 17:44:15]

lgtm

[Yang, 2017-02-09 19:04:00]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2017-02-09 19:04:01]

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/2686063002/#ps20001
(title: "address comments")

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

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:22:58]

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

[commit-bot: I haz the power, 2017-02-09 19:22:59]

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-10 07:28:29]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2017-02-10 07:28:29]

The patchset sent to the CQ was uploaded after l-g-t-m from hpayer@chromium.org,
jgruber@chromium.org
Link to the patchset: https://codereview.chromium.org/2686063002/#ps40001
(title: "fix flag")

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

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-10 08:21:04]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486711709363550,
"parent_rev": "a4f4c7405064cf31e75d57a921f012abfebe1bfd", "commit_rev":
"8422e25bb26449e0cf9831adafec48ee4e592278"}

[commit-bot: I haz the power, 2017-02-10 08:21:08]

Description was changed from

==========
[debugger] add precise mode for code coverage.

Collecting precise invocation counts need to be explicitly
enabled. Once enabled, we disable optimization (optimized
code does not increment invocation count, and may inline
callees), and make sure feedback vectors interesting for
code coverage is not garbage-collected.

R=hpayer@chromium.org, jgruber@chromium.org
BUG=v8:5808
==========

to

==========
[debugger] add precise mode for code coverage.

Collecting precise invocation counts need to be explicitly
enabled. Once enabled, we disable optimization (optimized
code does not increment invocation count, and may inline
callees), and make sure feedback vectors interesting for
code coverage is not garbage-collected.

R=hpayer@chromium.org, jgruber@chromium.org
BUG=v8:5808

Review-Url: https://codereview.chromium.org/2686063002
Cr-Commit-Position: refs/heads/master@{#43082}
Committed:
https://chromium.googlesource.com/v8/v8/+/8422e25bb26449e0cf9831adafec48ee4e5...
==========

[commit-bot: I haz the power, 2017-02-10 08:21:10]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/8422e25bb26449e0cf9831adafec48ee4e5...