[RuntimeCallStats] Move tracing runtime instrumentation closer to the original version.

[RuntimeCallStats] Move tracing runtime instrumentation closer to the original version.

After we landed the tracing runtime call stats, which gave
us a lot of V8 insight in tracing, we noticed that there is
some arising issues and discrepancies.

Issues include:
Missing trace events, that happened due to
transforming those trace events into runtime calls

Discrepancies include:
Missing categories in Runtime call stats like GC,
because we are not handling the Scoped runtime calls
properly in the tracing version.

To reduce/eliminate those issue, we are taking a small
step back. We are unifying the RuntimeStats code and
using the original one. That would allow us to use all
the original probes but emit trace events from them.
We are also putting back the trace-events in their place.

The output from both system should be intact (Except of
the addition of the missing trace-events).

Also as a byproduct, we are reducing the number of context
scopes by half since we are using the same scope as
runtime call stats.

As a follow up to this CL, we will address the non-scoped
Runtime Call Stats (mainly in GC).
BUG=642373
Committed: https://crrev.com/e5ba156d887e08f3ae3d29aa35e669f19a7dcb97
Cr-Commit-Position: refs/heads/master@{#39180}

[fmeawad, 2016-09-01 05:13:49]

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

[commit-bot: I haz the power, 2016-09-01 05:13: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, 2016-09-01 05:17:23]

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

[commit-bot: I haz the power, 2016-09-01 05:17:24]

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

[fmeawad, 2016-09-01 14:37:50]

Description was changed from

==========
[RuntimeCallStats] Performance Optimizations and other fixes

BUG=642373
==========

to

==========
[RuntimeCallStats] Performance Optimizations and other fixes

BUG=642373
==========

[fmeawad, 2016-09-01 21:26:41]

Description was changed from

==========
[RuntimeCallStats] Performance Optimizations and other fixes

BUG=642373
==========

to

==========
[RuntimeCallStats] Move tracing runtime instrumentation closer to the original
version.

After we landed the tracing runtime call stats, which gave
us a lot of V8 insight in tracing, we noticed that there is
some arising issues and discrepancies.

Issues include:
Missing trace events, that happened due to
transforming those trace events into runtime calls

Discrepancies include:
Missing categories in Runtime call stats like GC,
because we are not handling the Scoped runtime calls
properly in the tracing version.

To reduce/eliminate those issue, we are taking a small
step back. We are unifying the RuntimeStats code and
using the original one. That would allow us to use all
the original probes but emit trace events from them.
We are also putting back the trace-events in their place.

The output from both system should be intact (Except of
the addition of the missing trace-events).

As a follow up to this CL, we will address the non-scoped
Runtime Call Stats (mainly in GC).
BUG=642373
==========

[fmeawad, 2016-09-01 21:26:41]

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

[fmeawad, 2016-09-01 21:26:50]

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

[commit-bot: I haz the power, 2016-09-01 21:26:52]

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

[fmeawad, 2016-09-01 21:27:04]

lpy: Can you take an initial look?

[commit-bot: I haz the power, 2016-09-01 21:53:02]

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

[commit-bot: I haz the power, 2016-09-01 21:53:03]

Dry run: This issue passed the CQ dry run.

[fmeawad, 2016-09-01 21:56:11]

Description was changed from

==========
[RuntimeCallStats] Move tracing runtime instrumentation closer to the original
version.

After we landed the tracing runtime call stats, which gave
us a lot of V8 insight in tracing, we noticed that there is
some arising issues and discrepancies.

Issues include:
Missing trace events, that happened due to
transforming those trace events into runtime calls

Discrepancies include:
Missing categories in Runtime call stats like GC,
because we are not handling the Scoped runtime calls
properly in the tracing version.

To reduce/eliminate those issue, we are taking a small
step back. We are unifying the RuntimeStats code and
using the original one. That would allow us to use all
the original probes but emit trace events from them.
We are also putting back the trace-events in their place.

The output from both system should be intact (Except of
the addition of the missing trace-events).

As a follow up to this CL, we will address the non-scoped
Runtime Call Stats (mainly in GC).
BUG=642373
==========

to

==========
[RuntimeCallStats] Move tracing runtime instrumentation closer to the original
version.

After we landed the tracing runtime call stats, which gave
us a lot of V8 insight in tracing, we noticed that there is
some arising issues and discrepancies.

Issues include:
Missing trace events, that happened due to
transforming those trace events into runtime calls

Discrepancies include:
Missing categories in Runtime call stats like GC,
because we are not handling the Scoped runtime calls
properly in the tracing version.

To reduce/eliminate those issue, we are taking a small
step back. We are unifying the RuntimeStats code and
using the original one. That would allow us to use all
the original probes but emit trace events from them.
We are also putting back the trace-events in their place.

The output from both system should be intact (Except of
the addition of the missing trace-events).

Also as a byproduct, we are reducing the number of reducing
the number of context scopes by half since we are using
the same scope as runtime call stats.

As a follow up to this CL, we will address the non-scoped
Runtime Call Stats (mainly in GC).
BUG=642373
==========

[lpy, 2016-09-01 22:46:06]

LGTM with one comment.

https://codereview.chromium.org/2296243002/diff/100001/src/tracing/trace-event.h
File src/tracing/trace-event.h (right):

https://codereview.chromium.org/2296243002/diff/100001/src/tracing/trace-even...
src/tracing/trace-event.h:14: #include "src/isolate.h"
Can we make Isolate a forward declaration below in the namespace since we are
also including src/isolate.h in trace-event.cc?

[fmeawad, 2016-09-01 22:56:33]

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

[commit-bot: I haz the power, 2016-09-01 22:56:39]

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

[fmeawad, 2016-09-01 22:57:46]

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

[fmeawad, 2016-09-01 22:57:47]

cbruni, PTAL.

https://codereview.chromium.org/2296243002/diff/100001/src/tracing/trace-event.h
File src/tracing/trace-event.h (right):

https://codereview.chromium.org/2296243002/diff/100001/src/tracing/trace-even...
src/tracing/trace-event.h:14: #include "src/isolate.h"
On 2016/09/01 22:46:06, lpy wrote:
> Can we make Isolate a forward declaration below in the namespace since we are
> also including src/isolate.h in trace-event.cc?

Good Catch!
Done.

[commit-bot: I haz the power, 2016-09-02 00:01:09]

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

[commit-bot: I haz the power, 2016-09-02 00:01:11]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2016-09-02 00:12:46]

Description was changed from

==========
[RuntimeCallStats] Move tracing runtime instrumentation closer to the original
version.

After we landed the tracing runtime call stats, which gave
us a lot of V8 insight in tracing, we noticed that there is
some arising issues and discrepancies.

Issues include:
Missing trace events, that happened due to
transforming those trace events into runtime calls

Discrepancies include:
Missing categories in Runtime call stats like GC,
because we are not handling the Scoped runtime calls
properly in the tracing version.

To reduce/eliminate those issue, we are taking a small
step back. We are unifying the RuntimeStats code and
using the original one. That would allow us to use all
the original probes but emit trace events from them.
We are also putting back the trace-events in their place.

The output from both system should be intact (Except of
the addition of the missing trace-events).

Also as a byproduct, we are reducing the number of reducing
the number of context scopes by half since we are using
the same scope as runtime call stats.

As a follow up to this CL, we will address the non-scoped
Runtime Call Stats (mainly in GC).
BUG=642373
==========

to

==========
[RuntimeCallStats] Move tracing runtime instrumentation closer to the original
version.

After we landed the tracing runtime call stats, which gave
us a lot of V8 insight in tracing, we noticed that there is
some arising issues and discrepancies.

Issues include:
Missing trace events, that happened due to
transforming those trace events into runtime calls

Discrepancies include:
Missing categories in Runtime call stats like GC,
because we are not handling the Scoped runtime calls
properly in the tracing version.

To reduce/eliminate those issue, we are taking a small
step back. We are unifying the RuntimeStats code and
using the original one. That would allow us to use all
the original probes but emit trace events from them.
We are also putting back the trace-events in their place.

The output from both system should be intact (Except of
the addition of the missing trace-events).

Also as a byproduct, we are reducing the number of context
scopes by half since we are using the same scope as
runtime call stats.

As a follow up to this CL, we will address the non-scoped
Runtime Call Stats (mainly in GC).
BUG=642373
==========

[Camillo Bruni, 2016-09-02 00:30:50]

LGTM with nits.
You can address my comments in a follow-up CL.

https://codereview.chromium.org/2296243002/diff/120001/src/counters.h
File src/counters.h (right):

https://codereview.chromium.org/2296243002/diff/120001/src/counters.h#newcode788
src/counters.h:788: const char* Dump();
nit: Please mark the 2 methods above with V8_NOINLINE otherwise we get very
large methods (>100KB) due all the expanded macros

https://codereview.chromium.org/2296243002/diff/120001/src/counters.h#newcode...
src/counters.h:1280: RuntimeCallTimer trace_event_timer_;
I think we can make --runtime-call-stats and tracing mutually exclusive with
tracing taking precedence if both modes are on.
I don't see a use-case where --runtime-call-stats would give an advantage when
tracing is enabled.
The destructor check would then be:

if (V8_UNLIKELY(isolate_ != nullptr)) {
  if (FLAG_runtime_call_stats) { ...
  } else { ...
  }
}

[fmeawad, 2016-09-02 18:34:20]

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

[fmeawad, 2016-09-02 18:34:21]

Comments addressed, 

Adding reviewers:
bmeurer: src/full-codegen, src/interpreter
ulan: src/heap
adamk: src/parsing

PTAL.

https://codereview.chromium.org/2296243002/diff/120001/src/counters.h
File src/counters.h (right):

https://codereview.chromium.org/2296243002/diff/120001/src/counters.h#newcode788
src/counters.h:788: const char* Dump();
On 2016/09/02 00:30:49, Camillo Bruni wrote:
> nit: Please mark the 2 methods above with V8_NOINLINE otherwise we get very
> large methods (>100KB) due all the expanded macros

Done.

https://codereview.chromium.org/2296243002/diff/120001/src/counters.h#newcode...
src/counters.h:1280: RuntimeCallTimer trace_event_timer_;
On 2016/09/02 00:30:50, Camillo Bruni wrote:
> I think we can make --runtime-call-stats and tracing mutually exclusive with
> tracing taking precedence if both modes are on.
> I don't see a use-case where --runtime-call-stats would give an advantage when
> tracing is enabled.
> The destructor check would then be:
> 
> if (V8_UNLIKELY(isolate_ != nullptr)) {
>   if (FLAG_runtime_call_stats) { ...
>   } else { ...
>   }
> }

Added a comment to make a followup CL

[fmeawad, 2016-09-02 18:34:58]

fmeawad@chromium.org changed reviewers:
+ adamk@chromium.org, ulan@chromium.org

[fmeawad, 2016-09-02 18:34:58]

(Actually adding the reviewers)

[fmeawad, 2016-09-02 18:35:09]

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

[commit-bot: I haz the power, 2016-09-02 18:35:15]

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

[adamk, 2016-09-02 18:36:25]

lgtm for parsing/

[commit-bot: I haz the power, 2016-09-02 18:38:25]

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

[commit-bot: I haz the power, 2016-09-02 18:38:26]

Dry run: Try jobs failed on following builders:
  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_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)

[fmeawad, 2016-09-02 19:23:48]

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

[commit-bot: I haz the power, 2016-09-02 19:23: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, 2016-09-02 19:49:59]

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

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

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2016-09-03 17:56:34]

LGTM on fullcodegen and interpreter.

[ulan, 2016-09-05 07:48:16]

heap lgtm

[fmeawad, 2016-09-05 15:13:26]

The CQ bit was checked by fmeawad@chromium.org

[fmeawad, 2016-09-05 15:13:27]

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

[commit-bot: I haz the power, 2016-09-05 15:13:33]

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-09-05 15:39:44]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-09-05 15:40:20]

Description was changed from

==========
[RuntimeCallStats] Move tracing runtime instrumentation closer to the original
version.

After we landed the tracing runtime call stats, which gave
us a lot of V8 insight in tracing, we noticed that there is
some arising issues and discrepancies.

Issues include:
Missing trace events, that happened due to
transforming those trace events into runtime calls

Discrepancies include:
Missing categories in Runtime call stats like GC,
because we are not handling the Scoped runtime calls
properly in the tracing version.

To reduce/eliminate those issue, we are taking a small
step back. We are unifying the RuntimeStats code and
using the original one. That would allow us to use all
the original probes but emit trace events from them.
We are also putting back the trace-events in their place.

The output from both system should be intact (Except of
the addition of the missing trace-events).

Also as a byproduct, we are reducing the number of context
scopes by half since we are using the same scope as
runtime call stats.

As a follow up to this CL, we will address the non-scoped
Runtime Call Stats (mainly in GC).
BUG=642373
==========

to

==========
[RuntimeCallStats] Move tracing runtime instrumentation closer to the original
version.

After we landed the tracing runtime call stats, which gave
us a lot of V8 insight in tracing, we noticed that there is
some arising issues and discrepancies.

Issues include:
Missing trace events, that happened due to
transforming those trace events into runtime calls

Discrepancies include:
Missing categories in Runtime call stats like GC,
because we are not handling the Scoped runtime calls
properly in the tracing version.

To reduce/eliminate those issue, we are taking a small
step back. We are unifying the RuntimeStats code and
using the original one. That would allow us to use all
the original probes but emit trace events from them.
We are also putting back the trace-events in their place.

The output from both system should be intact (Except of
the addition of the missing trace-events).

Also as a byproduct, we are reducing the number of context
scopes by half since we are using the same scope as
runtime call stats.

As a follow up to this CL, we will address the non-scoped
Runtime Call Stats (mainly in GC).
BUG=642373

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

[commit-bot: I haz the power, 2016-09-05 15:40:21]

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

[fmeawad, 2016-09-05 21:32:57]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/2311983002/ by fmeawad@chromium.org.

The reason for reverting is: There is a wrong scoping check making the tracing
version not working..