[counters] Implement off-isolate RuntimeCallStats for the Preparser

[counters] Implement off-isolate RuntimeCallStats for the Preparser

BUG=

Committed: https://crrev.com/bb6a626b7634cc5e9258e6c7bd0eb0b734739503
Cr-Commit-Position: refs/heads/master@{#41001}

[Camillo Bruni, 2016-11-08 16:13:09]

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

[commit-bot: I haz the power, 2016-11-08 16:13:10]

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-11-08 16:14:40]

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

[commit-bot: I haz the power, 2016-11-08 16:14:41]

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_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_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_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/15827)
  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_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/15866)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/15789)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/28082)

[Camillo Bruni, 2016-11-09 14:15:18]

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

[Camillo Bruni, 2016-11-09 14:15:19]

PTAL

[Camillo Bruni, 2016-11-09 14:15:22]

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

[Camillo Bruni, 2016-11-09 14:17:33]

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

[commit-bot: I haz the power, 2016-11-09 14:17:42]

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

[Camillo Bruni, 2016-11-09 14:33:47]

Description was changed from

==========
[counters] Implement off-thread RuntimeCallStats

BUG=
==========

to

==========
[counters] Implement off-isolate RuntimeCallStats for the Preparser

BUG=
==========

[commit-bot: I haz the power, 2016-11-09 14:46:31]

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

[commit-bot: I haz the power, 2016-11-09 14:46:31]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2016-11-09 15:28:26]

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

[commit-bot: I haz the power, 2016-11-09 15:28:29]

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-11-09 15:54:15]

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

[commit-bot: I haz the power, 2016-11-09 15:54:16]

Dry run: This issue passed the CQ dry run.

[lpy, 2016-11-09 16:47:42]

Just nits.

https://codereview.chromium.org/2490643002/diff/80001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2490643002/diff/80001/src/parsing/parser.cc#n...
src/parsing/parser.cc:3827: if (FLAG_runtime_call_stats) {
How about using FLAG_runtime_stats here? I prefer us to avoid using
FLAG_runtime_call_stats.

https://codereview.chromium.org/2490643002/diff/80001/src/parsing/parser.cc#n...
src/parsing/parser.cc:3870: if (FLAG_runtime_call_stats) {
Same here.

[Camillo Bruni, 2016-11-10 10:49:40]

cbruni@chromium.org changed reviewers:
+ vogelheim@chromium.org
- verwaest@chromium.org

[Camillo Bruni, 2016-11-10 10:49:40]

PTAL

https://codereview.chromium.org/2490643002/diff/80001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2490643002/diff/80001/src/parsing/parser.cc#n...
src/parsing/parser.cc:3827: if (FLAG_runtime_call_stats) {
On 2016/11/09 at 16:47:42, lpy wrote:
> How about using FLAG_runtime_stats here? I prefer us to avoid using
FLAG_runtime_call_stats.

done.

https://codereview.chromium.org/2490643002/diff/80001/src/parsing/parser.cc#n...
src/parsing/parser.cc:3870: if (FLAG_runtime_call_stats) {
On 2016/11/09 at 16:47:42, lpy wrote:
> Same here.

done.

[fmeawad, 2016-11-10 15:50:59]

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

[fmeawad, 2016-11-10 15:51:00]

Runtime Stats: lgtm.

lpy@ can you follow up on this CL to make sure it the data shows up properly on
the background parser thread (ScriptStreamer) in trace-viewer

https://codereview.chromium.org/2490643002/diff/100001/src/counters.cc
File src/counters.cc (right):

https://codereview.chromium.org/2490643002/diff/100001/src/counters.cc#newcod...
src/counters.cc:353: while (current < &(this->Last)) {
Is there a performance impact from adding a check here? I guess we only call
reset at the end of top level events only, so it should be OK.

[Camillo Bruni, 2016-11-11 12:17:35]

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

[commit-bot: I haz the power, 2016-11-11 12:17:48]

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-11-11 12:19:36]

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

[commit-bot: I haz the power, 2016-11-11 12:19: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_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_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/16070)
  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_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/16112)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/16031)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/12377)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/28368)

[Camillo Bruni, 2016-11-11 12:38:50]

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

[commit-bot: I haz the power, 2016-11-11 12:39: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-11-11 13:05:24]

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

[commit-bot: I haz the power, 2016-11-11 13:05:25]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2016-11-14 09:38:56]

https://codereview.chromium.org/2490643002/diff/100001/src/counters.cc
File src/counters.cc (right):

https://codereview.chromium.org/2490643002/diff/100001/src/counters.cc#newcod...
src/counters.cc:353: while (current < &(this->Last)) {
On 2016/11/10 at 15:51:00, fmeawad wrote:
> Is there a performance impact from adding a check here? I guess we only call
reset at the end of top level events only, so it should be OK.

I think this doesn't really matter speed-wise, we do, however, have a code-size
issue if we inline too much here (and we've had complaints in the past already
:P)

[vogelheim, 2016-11-14 10:20:33]

Generally looks good, but some nitpicks, plus one thing I'm concerned about (the
background/race-condition thing in parser.cc). I might have misunderstood that
bit though, so please let me know what you think.

https://codereview.chromium.org/2490643002/diff/120001/src/counters.cc
File src/counters.cc (right):

https://codereview.chromium.org/2490643002/diff/120001/src/counters.cc#newcod...
src/counters.cc:316: RuntimeCallCounter* current = &(this->First);
(here and below):

I take it you use First & Last counters only to be able to iterate over all your
counters. But, as I read it, unlike begin() + end() your First points to
*before* the counter range; it's effectively begin() - 1. If so, I'd expect all
your iterations to start from First() + 1.

[No big deal; I take it the additional First() is harmless. But it's certainly
weird.]

https://codereview.chromium.org/2490643002/diff/120001/src/counters.cc#newcod...
src/counters.cc:322: reinterpret_cast<intptr_t>(other_counter) +
sizeof(RuntimeCallCounter));
A nitpick, loosely related to the comment above: These reinterpret_casts +
pointer arithmetic seems quite gratuitous. I take it this is because they're
declared as regular members, and now we want to iterate over the members of a
class, which C++ doesn't provide for. Maybe we can change the declaration to put
all of the RuntimeCallCounters into an array?

https://codereview.chromium.org/2490643002/diff/120001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2490643002/diff/120001/src/parsing/parser.cc#...
src/parsing/parser.cc:669: // see comment for HistogramTimerScope class.
[Not yours, but..] Is this comment/TODO still accurate? I take it the
HistogramTimerScope was once in the place that RuntimeCallTimerScope is now? Or
did that morph into the TRACE_EVENT?

https://codereview.chromium.org/2490643002/diff/120001/src/parsing/parser.cc#...
src/parsing/parser.cc:3877:
default_runtime_call_stats->Add(runtime_call_stats_);
Hmm. I'm not sure, but aren't we still on the background thread when arriving
here? If so, this would be a rather big race condition...

Maybe we could do this work when Parser::Internalize happens? That's the logic
point where we reconcile the background parse data with the main
thread-accessible runtime state.

[Camillo Bruni, 2016-11-14 12:43:46]

PTAL again

https://codereview.chromium.org/2490643002/diff/120001/src/counters.cc
File src/counters.cc (right):

https://codereview.chromium.org/2490643002/diff/120001/src/counters.cc#newcod...
src/counters.cc:316: RuntimeCallCounter* current = &(this->First);
On 2016/11/14 at 10:20:33, vogelheim wrote:
> (here and below):
> 
> I take it you use First & Last counters only to be able to iterate over all
your counters. But, as I read it, unlike begin() + end() your First points to
*before* the counter range; it's effectively begin() - 1. If so, I'd expect all
your iterations to start from First() + 1.
> 
> [No big deal; I take it the additional First() is harmless. But it's certainly
weird.]

Yes, that's right, given that First and Last are just dummy counters it dosn't
matter.
I can add a comment to make this assumption obvious.

https://codereview.chromium.org/2490643002/diff/120001/src/counters.cc#newcod...
src/counters.cc:322: reinterpret_cast<intptr_t>(other_counter) +
sizeof(RuntimeCallCounter));
On 2016/11/14 at 10:20:33, vogelheim wrote:
> A nitpick, loosely related to the comment above: These reinterpret_casts +
pointer arithmetic seems quite gratuitous. I take it this is because they're
declared as regular members, and now we want to iterate over the members of a
class, which C++ doesn't provide for. Maybe we can change the declaration to put
all of the RuntimeCallCounters into an array?

Right, I had some discussions around this as it is far from pretty the way we do
it now.
Having an array would probably clean this up at the cost of yet another pointer
indirection to access the current timer.

https://codereview.chromium.org/2490643002/diff/120001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2490643002/diff/120001/src/parsing/parser.cc#...
src/parsing/parser.cc:3877:
default_runtime_call_stats->Add(runtime_call_stats_);
On 2016/11/14 at 10:20:33, vogelheim wrote:
> Hmm. I'm not sure, but aren't we still on the background thread when arriving
here? If so, this would be a rather big race condition...
> 
> Maybe we could do this work when Parser::Internalize happens? That's the logic
point where we reconcile the background parse data with the main
thread-accessible runtime state.

Indeed a better place.

[vogelheim, 2016-11-14 13:05:05]

lgtm. thanks!

https://codereview.chromium.org/2490643002/diff/140001/src/counters.h
File src/counters.h (right):

https://codereview.chromium.org/2490643002/diff/140001/src/counters.h#newcode737
src/counters.h:737: V(PreParseArrowFunctionLiteral)                   \
nitpick: The list was (mostly) alphabetically ordered, but you seem to use a
non-standard alphabet.

 ParseEval < ParseProgram
 ParseFunction < ParseProgram  [this one was in already]
 PreParseNo... < PreParseWith
 ParseArr... < ParseEval

https://codereview.chromium.org/2490643002/diff/140001/src/counters.h#newcode854
src/counters.h:854: FOR_EACH_HANDLER_COUNTER(CALL_BUILTIN_COUNTER)
nitpick: The formatting is weird, with the ever-increasing continuation-line
indent. I've seen clang-format do that when there's too many #-linesm in which
cases I just manually formatted it. (Admittedly, I'm not sure this still works
w/ our pre-submit checks.)

[Camillo Bruni, 2016-11-14 14:01:37]

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

[commit-bot: I haz the power, 2016-11-14 14:01:44]

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-11-14 14:06:38]

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

[commit-bot: I haz the power, 2016-11-14 14:06:39]

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

[Camillo Bruni, 2016-11-15 15:32:22]

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

[commit-bot: I haz the power, 2016-11-15 15:32: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, 2016-11-15 15:59:56]

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

[commit-bot: I haz the power, 2016-11-15 15:59:57]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2016-11-15 16:05:46]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-11-15 16:05:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
fmeawad@chromium.org, vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2490643002/#ps240001
(title: "fixing declaration")

[commit-bot: I haz the power, 2016-11-15 16:06:00]

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-11-15 16:08:23]

Committed patchset #13 (id:240001)

[Camillo Bruni, 2016-11-15 18:10:29]

https://codereview.chromium.org/2490643002/diff/140001/src/counters.h
File src/counters.h (right):

https://codereview.chromium.org/2490643002/diff/140001/src/counters.h#newcode737
src/counters.h:737: V(PreParseArrowFunctionLiteral)                   \
On 2016/11/14 at 13:05:05, vogelheim wrote:
> nitpick: The list was (mostly) alphabetically ordered, but you seem to use a
non-standard alphabet.
> 
>  ParseEval < ParseProgram
>  ParseFunction < ParseProgram  [this one was in already]
>  PreParseNo... < PreParseWith
>  ParseArr... < ParseEval

argh, thought that I did sort them, thx!

https://codereview.chromium.org/2490643002/diff/140001/src/counters.h#newcode854
src/counters.h:854: FOR_EACH_HANDLER_COUNTER(CALL_BUILTIN_COUNTER)
On 2016/11/14 at 13:05:05, vogelheim wrote:
> nitpick: The formatting is weird, with the ever-increasing continuation-line
indent. I've seen clang-format do that when there's too many #-linesm in which
cases I just manually formatted it. (Admittedly, I'm not sure this still works
w/ our pre-submit checks.)

I tried a couple of times to force a different layout... presubmit is evil :)

[commit-bot: I haz the power, 2016-11-17 22:34:33]

Description was changed from

==========
[counters] Implement off-isolate RuntimeCallStats for the Preparser

BUG=
==========

to

==========
[counters] Implement off-isolate RuntimeCallStats for the Preparser

BUG=

Committed: https://crrev.com/bb6a626b7634cc5e9258e6c7bd0eb0b734739503
Cr-Commit-Position: refs/heads/master@{#41001}
==========

[commit-bot: I haz the power, 2016-11-17 22:34:34]

Patchset 13 (id:??) landed as
https://crrev.com/bb6a626b7634cc5e9258e6c7bd0eb0b734739503
Cr-Commit-Position: refs/heads/master@{#41001}