[Compiler] Track Ignition background compilation separately in RuntimeStats.

[Compiler] Track Ignition background compilation separately in RuntimeStats.

Tracks background compilation of Ignition in a separate bucket from main thread
compilation. Also add some more compilation buckets for functions which can take a
significant proportion of compilation.

BUG=v8:5203, v8:5215
Review-Url: https://codereview.chromium.org/2577263002
Cr-Original-Commit-Position: refs/heads/master@{#42026}
Committed: https://chromium.googlesource.com/v8/v8/+/b0e9116d59326a4f9a4f4691b61a510e342338c9
Review-Url: https://codereview.chromium.org/2577263002
Cr-Commit-Position: refs/heads/master@{#42042}
Committed: https://chromium.googlesource.com/v8/v8/+/c89921258754569147258c33175e9687b1fe4bd2

[rmcilroy, 2016-12-15 23:19:19]

Patchset #1 (id:1) has been deleted

[rmcilroy, 2016-12-15 23:19:23]

Patchset #1 (id:20001) has been deleted

[rmcilroy, 2016-12-16 00:09:54]

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

[commit-bot: I haz the power, 2016-12-16 00:10:00]

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, 2016-12-16 00:39:41]

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

[commit-bot: I haz the power, 2016-12-16 00:39:42]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-12-16 07:47:23]

Description was changed from

==========
[Compiler] Track Ignition background compilation seperatly in RuntimeStats.

Tracks background compilation of Ignition in a seperate bucket from main thread
compilation. Also add some more compilation buckets for functions which can take
a
significant proportion of compilation.

BUG=v8:5203,v8:5215
==========

to

==========
[Compiler] Track Ignition background compilation separately in RuntimeStats.

Tracks background compilation of Ignition in a separate bucket from main thread
compilation. Also add some more compilation buckets for functions which can take
a
significant proportion of compilation.

BUG=v8:5203,v8:5215
==========

[rmcilroy, 2016-12-16 07:47:39]

rmcilroy@chromium.org changed reviewers:
+ cbruni@chromium.org

[rmcilroy, 2016-12-16 07:47:39]

Camillo, PTAL thanks.

[rmcilroy, 2016-12-16 08:33:55]

+Jochen for FYI

[Camillo Bruni, 2016-12-16 09:25:05]

https://codereview.chromium.org/2577263002/diff/40001/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/2577263002/diff/40001/src/compiler.h#newcode197
src/compiler.h:197: !executed_on_background_thread_);
DCHECK_IMPLIES(!can_execute_on_background_thread(),
!executed_on_background_thread_)

https://codereview.chromium.org/2577263002/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2577263002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:190:
TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.compile"), "V8.CompileIgnition");
Please add: TODO(lpy): add support for background compilation RCS trace
see https://codereview.chromium.org/2559403002

https://codereview.chromium.org/2577263002/diff/40001/tools/callstats.py
File tools/callstats.py (right):

https://codereview.chromium.org/2577263002/diff/40001/tools/callstats.py#newc...
tools/callstats.py:349: ('Group-CompileBackground',
re.compile("(.*CompileBackground.*)")),
Could you add the same regexp entry in callstats.html (it has a similar section,
and choose your favorite color for the graphs ;)

[jochen (gone - plz use gerrit), 2016-12-16 10:47:37]

jochen@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-12-16 10:47:38]

https://codereview.chromium.org/2577263002/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2577263002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:184: runtime_call_stats_ = new (info()->zone())
RuntimeCallStats();
this allocates a potentially pretty large object even though we just need one
counter. Should we rather just instantiate the one counter?

[Camillo Bruni, 2016-12-16 11:42:54]

https://codereview.chromium.org/2577263002/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2577263002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:184: runtime_call_stats_ = new (info()->zone())
RuntimeCallStats();
On 2016/12/16 at 10:47:38, jochen wrote:
> this allocates a potentially pretty large object even though we just need one
counter. Should we rather just instantiate the one counter?

I think we initially discussed to do exactly that, using a separate timer and
directly sum up the values in the correct counter
(RuntimeCallStats::CompileBackgroundIgnition).

[rmcilroy, 2016-12-16 23:48:05]

https://codereview.chromium.org/2577263002/diff/40001/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/2577263002/diff/40001/src/compiler.h#newcode197
src/compiler.h:197: !executed_on_background_thread_);
On 2016/12/16 09:25:05, Camillo Bruni wrote:
> DCHECK_IMPLIES(!can_execute_on_background_thread(),
> !executed_on_background_thread_)

Done.

https://codereview.chromium.org/2577263002/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2577263002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:184: runtime_call_stats_ = new (info()->zone())
RuntimeCallStats();
On 2016/12/16 11:42:54, Camillo Bruni wrote:
> On 2016/12/16 at 10:47:38, jochen wrote:
> > this allocates a potentially pretty large object even though we just need
one
> counter. Should we rather just instantiate the one counter?
> 
> I think we initially discussed to do exactly that, using a separate timer and
> directly sum up the values in the correct counter
> (RuntimeCallStats::CompileBackgroundIgnition).

Yeah good point. I was basing this on the approach taken by the background
parser, but have changed to use a separate counter (with a special TimerScope to
ensure we still correctly nest timers if running on the main thread). If we are
worried about allocating a large object on every compile then we should probably
adapt the approach taken in the parser too, since it will end up running as
often as this code with the CompilerDispatcher.

https://codereview.chromium.org/2577263002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:190:
TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.compile"), "V8.CompileIgnition");
On 2016/12/16 09:25:05, Camillo Bruni wrote:
> Please add: TODO(lpy): add support for background compilation RCS trace
> see https://codereview.chromium.org/2559403002

Done.

https://codereview.chromium.org/2577263002/diff/40001/tools/callstats.py
File tools/callstats.py (right):

https://codereview.chromium.org/2577263002/diff/40001/tools/callstats.py#newc...
tools/callstats.py:349: ('Group-CompileBackground',
re.compile("(.*CompileBackground.*)")),
On 2016/12/16 09:25:05, Camillo Bruni wrote:
> Could you add the same regexp entry in callstats.html (it has a similar
section,
> and choose your favorite color for the graphs ;)

Done.

[Camillo Bruni, 2016-12-19 07:34:59]

lgtm

[Camillo Bruni, 2016-12-19 07:35:39]

On 2016/12/16 at 23:48:05, rmcilroy wrote:
> https://codereview.chromium.org/2577263002/diff/40001/src/compiler.h
> File src/compiler.h (right):
> 
>
https://codereview.chromium.org/2577263002/diff/40001/src/compiler.h#newcode197
> src/compiler.h:197: !executed_on_background_thread_);
> On 2016/12/16 09:25:05, Camillo Bruni wrote:
> > DCHECK_IMPLIES(!can_execute_on_background_thread(),
> > !executed_on_background_thread_)
> 
> Done.
> 
>
https://codereview.chromium.org/2577263002/diff/40001/src/interpreter/interpr...
> File src/interpreter/interpreter.cc (right):
> 
>
https://codereview.chromium.org/2577263002/diff/40001/src/interpreter/interpr...
> src/interpreter/interpreter.cc:184: runtime_call_stats_ = new (info()->zone())
RuntimeCallStats();
> On 2016/12/16 11:42:54, Camillo Bruni wrote:
> > On 2016/12/16 at 10:47:38, jochen wrote:
> > > this allocates a potentially pretty large object even though we just need
one
> > counter. Should we rather just instantiate the one counter?
> > 
> > I think we initially discussed to do exactly that, using a separate timer
and
> > directly sum up the values in the correct counter
> > (RuntimeCallStats::CompileBackgroundIgnition).
> 
> Yeah good point. I was basing this on the approach taken by the background
parser, but have changed to use a separate counter (with a special TimerScope to
ensure we still correctly nest timers if running on the main thread). If we are
worried about allocating a large object on every compile then we should probably
adapt the approach taken in the parser too, since it will end up running as
often as this code with the CompilerDispatcher.

Ok, probably fine :), let's see how well this performs...

[jochen (gone - plz use gerrit), 2017-01-02 07:28:14]

here's another thought: what about making the counting stuff atomic? That way,
we wouldn't need an explicit point on the main thread, where the background
counters are added.

[rmcilroy, 2017-01-03 09:00:44]

On 2017/01/02 07:28:14, jochen wrote:
> here's another thought: what about making the counting stuff atomic? That way,
> we wouldn't need an explicit point on the main thread, where the background
> counters are added.

That would work, but you would need a CAS plus associated check and retry logic
when updating the table, which might be too expensive for entries which are all
on the main thread. Camillo, thoughts?

I'll land this for now, and we can update to atomics if it isn't too expensive
elsewhere.

[jochen (gone - plz use gerrit), 2017-01-03 09:02:41]

yeah, I'd use the atomics version only for counters that can be used from any
thread.

[rmcilroy, 2017-01-03 11:41:06]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2017-01-03 11:41:12]

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-01-03 11:42:21]

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

[commit-bot: I haz the power, 2017-01-03 11:42:22]

Try jobs failed on following builders:
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/14919)

[rmcilroy, 2017-01-03 12:21:35]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2017-01-03 12:21:35]

The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org
Link to the patchset: https://codereview.chromium.org/2577263002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-03 12:21:39]

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-01-03 12:49:17]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483446095340920,
"parent_rev": "b8f93dfe9d3b5c7fdb60ad1ab415bfd12f930e16", "commit_rev":
"b0e9116d59326a4f9a4f4691b61a510e342338c9"}

[commit-bot: I haz the power, 2017-01-03 12:49:21]

Description was changed from

==========
[Compiler] Track Ignition background compilation separately in RuntimeStats.

Tracks background compilation of Ignition in a separate bucket from main thread
compilation. Also add some more compilation buckets for functions which can take
a
significant proportion of compilation.

BUG=v8:5203,v8:5215
==========

to

==========
[Compiler] Track Ignition background compilation separately in RuntimeStats.

Tracks background compilation of Ignition in a separate bucket from main thread
compilation. Also add some more compilation buckets for functions which can take
a
significant proportion of compilation.

BUG=v8:5203,v8:5215

Review-Url: https://codereview.chromium.org/2577263002
Cr-Commit-Position: refs/heads/master@{#42026}
Committed:
https://chromium.googlesource.com/v8/v8/+/b0e9116d59326a4f9a4f4691b61a510e342...
==========

[commit-bot: I haz the power, 2017-01-03 12:49:22]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/v8/v8/+/b0e9116d59326a4f9a4f4691b61a510e342...

[Michael Achenbach, 2017-01-03 14:05:29]

A revert of this CL (patchset #3 id:80001) has been created in
https://codereview.chromium.org/2609773003/ by machenbach@chromium.org.

The reason for reverting is:
https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20TSAN/builds/13358.

[rmcilroy, 2017-01-03 14:52:53]

Description was changed from

==========
[Compiler] Track Ignition background compilation separately in RuntimeStats.

Tracks background compilation of Ignition in a separate bucket from main thread
compilation. Also add some more compilation buckets for functions which can take
a
significant proportion of compilation.

BUG=v8:5203,v8:5215

Review-Url: https://codereview.chromium.org/2577263002
Cr-Commit-Position: refs/heads/master@{#42026}
Committed:
https://chromium.googlesource.com/v8/v8/+/b0e9116d59326a4f9a4f4691b61a510e342...
==========

to

==========
[Compiler] Track Ignition background compilation separately in RuntimeStats.

Tracks background compilation of Ignition in a separate bucket from main thread
compilation. Also add some more compilation buckets for functions which can take
a
significant proportion of compilation.

BUG=v8:5203,v8:5215

Review-Url: https://codereview.chromium.org/2577263002
Cr-Commit-Position: refs/heads/master@{#42026}
Committed:
https://chromium.googlesource.com/v8/v8/+/b0e9116d59326a4f9a4f4691b61a510e342...
==========

[rmcilroy, 2017-01-03 15:21:52]

Relanding with a small tweak to fix TSAN.

[rmcilroy, 2017-01-03 15:22:22]

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

[commit-bot: I haz the power, 2017-01-03 15:22:26]

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-01-03 15:38:37]

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

[commit-bot: I haz the power, 2017-01-03 15:38:37]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[rmcilroy, 2017-01-03 16:02:45]

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

[commit-bot: I haz the power, 2017-01-03 16:02:53]

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-01-03 16:26:44]

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

[commit-bot: I haz the power, 2017-01-03 16:26:45]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/20149)
  v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)

[rmcilroy, 2017-01-03 17:11:23]

Patchset #5 (id:120001) has been deleted

[rmcilroy, 2017-01-03 17:11:39]

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

[commit-bot: I haz the power, 2017-01-03 17:11:44]

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

[jochen (gone - plz use gerrit), 2017-01-03 17:12:59]

lgtm

[commit-bot: I haz the power, 2017-01-03 17:37:46]

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

[commit-bot: I haz the power, 2017-01-03 17:37:48]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2017-01-03 18:10:10]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2017-01-03 18:10:10]

The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org
Link to the patchset: https://codereview.chromium.org/2577263002/#ps140001
(title: "Fix crash on accessing compilationinfo in constructor")

[commit-bot: I haz the power, 2017-01-03 18:10:21]

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-01-03 18:11:59]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1483467010159500,
"parent_rev": "db13377fe81313924e06c1e70207088daffe73ca", "commit_rev":
"c89921258754569147258c33175e9687b1fe4bd2"}

[commit-bot: I haz the power, 2017-01-03 18:12:03]

Description was changed from

==========
[Compiler] Track Ignition background compilation separately in RuntimeStats.

Tracks background compilation of Ignition in a separate bucket from main thread
compilation. Also add some more compilation buckets for functions which can take
a
significant proportion of compilation.

BUG=v8:5203,v8:5215

Review-Url: https://codereview.chromium.org/2577263002
Cr-Commit-Position: refs/heads/master@{#42026}
Committed:
https://chromium.googlesource.com/v8/v8/+/b0e9116d59326a4f9a4f4691b61a510e342...
==========

to

==========
[Compiler] Track Ignition background compilation separately in RuntimeStats.

Tracks background compilation of Ignition in a separate bucket from main thread
compilation. Also add some more compilation buckets for functions which can take
a
significant proportion of compilation.

BUG=v8:5203,v8:5215

Review-Url: https://codereview.chromium.org/2577263002
Cr-Original-Commit-Position: refs/heads/master@{#42026}
Committed:
https://chromium.googlesource.com/v8/v8/+/b0e9116d59326a4f9a4f4691b61a510e342...
Review-Url: https://codereview.chromium.org/2577263002
Cr-Commit-Position: refs/heads/master@{#42042}
Committed:
https://chromium.googlesource.com/v8/v8/+/c89921258754569147258c33175e9687b1f...
==========

[commit-bot: I haz the power, 2017-01-03 18:12:04]

Committed patchset #5 (id:140001) as
https://chromium.googlesource.com/v8/v8/+/c89921258754569147258c33175e9687b1f...