[Tracing] V8 Tracing Controller

[Tracing] V8 Tracing Controller

V8 has had a trace event macro interface for while, but without a tracing
controller a standalone V8 would be unable to collect traces.

This CL introduces a complete Tracing Controller system for V8.
It is fully function except that it does not yet store trace event args.

This CL has a few components,
The tracing controller itself, contributed by the author of this CL
The Trace config (including the parser), contributed by lpy@
The Trace Object, Trace Writer, and Trace Buffer are all contributed by rksang@

BUG=v8:4561
LOG=N

Committed: https://crrev.com/3d598452679ce208ad9b2f48e0fb3fae352ce375
Cr-Commit-Position: refs/heads/master@{#38073}

[fmeawad, 2016-07-12 00:36:02]

fmeawad@chromium.org changed reviewers:
+ mattloring@google.com, rskang@google.com

[fmeawad, 2016-07-12 00:36:02]

Matt/Ray/Peiyong: can you do a first pass review?

[fmeawad, 2016-07-12 00:37:14]

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

[fmeawad, 2016-07-12 00:37:53]

Description was changed from

==========
[Tracing] V8 Tracing Controller

V8 has had a trace event macro interface for while, but without a tracing
controller a standalone V8 would be unable to collect traces.

This CL introduces a complete Tracing Controller system for V8.
It is fully function except that it does not yet store trace event args.

This CL has a few components,
The tracing controller itself, contributed by the author of this CL
The Trace config (including the parser), contributed by lpy@
The Trace Object, Trace Writer, and Trace Buffer are all contributed by rksang@

(The first patch does not include the tests)

BUG=v8:4561
LOG=N
==========

to

==========
[Tracing] V8 Tracing Controller

V8 has had a trace event macro interface for while, but without a tracing
controller a standalone V8 would be unable to collect traces.

This CL introduces a complete Tracing Controller system for V8.
It is fully function except that it does not yet store trace event args.

This CL has a few components,
The tracing controller itself, contributed by the author of this CL
The Trace config (including the parser), contributed by lpy@
The Trace Object, Trace Writer, and Trace Buffer are all contributed by rksang@

(The first patch does not include the tests, and BUILD.gn changes)

BUG=v8:4561
LOG=N
==========

[rskang, 2016-07-12 00:58:52]

https://codereview.chromium.org/2137013006/diff/1/src/libplatform/tracing/tra...
File src/libplatform/tracing/trace-buffer.h (right):

https://codereview.chromium.org/2137013006/diff/1/src/libplatform/tracing/tra...
src/libplatform/tracing/trace-buffer.h:35: };
Can TraceBufferChunk go into v8-tracing.h as well? So that embedders
implementing new TraceBuffers can get access to it.

[fmeawad, 2016-07-12 21:03:00]

Description was changed from

==========
[Tracing] V8 Tracing Controller

V8 has had a trace event macro interface for while, but without a tracing
controller a standalone V8 would be unable to collect traces.

This CL introduces a complete Tracing Controller system for V8.
It is fully function except that it does not yet store trace event args.

This CL has a few components,
The tracing controller itself, contributed by the author of this CL
The Trace config (including the parser), contributed by lpy@
The Trace Object, Trace Writer, and Trace Buffer are all contributed by rksang@

(The first patch does not include the tests, and BUILD.gn changes)

BUG=v8:4561
LOG=N
==========

to

==========
[Tracing] V8 Tracing Controller

V8 has had a trace event macro interface for while, but without a tracing
controller a standalone V8 would be unable to collect traces.

This CL introduces a complete Tracing Controller system for V8.
It is fully function except that it does not yet store trace event args.

This CL has a few components,
The tracing controller itself, contributed by the author of this CL
The Trace config (including the parser), contributed by lpy@
The Trace Object, Trace Writer, and Trace Buffer are all contributed by rksang@

BUG=v8:4561
LOG=N
==========

[fmeawad, 2016-07-12 21:03:19]

https://codereview.chromium.org/2137013006/diff/1/src/libplatform/tracing/tra...
File src/libplatform/tracing/trace-buffer.h (right):

https://codereview.chromium.org/2137013006/diff/1/src/libplatform/tracing/tra...
src/libplatform/tracing/trace-buffer.h:35: };
On 2016/07/12 00:58:52, rskang wrote:
> Can TraceBufferChunk go into v8-tracing.h as well? So that embedders
> implementing new TraceBuffers can get access to it.

Done.

[rskang, 2016-07-13 00:54:32]

https://codereview.chromium.org/2137013006/diff/40001/include/libplatform/v8-...
File include/libplatform/v8-tracing.h (right):

https://codereview.chromium.org/2137013006/diff/40001/include/libplatform/v8-...
include/libplatform/v8-tracing.h:180: void Stop();
Can these (Initialize ... Stop) be made virtual as previously? In order to allow
embedders to extend TracingController and replace the methods.

[mattloring, 2016-07-15 00:19:01]

https://codereview.chromium.org/2137013006/diff/60001/src/libplatform/tracing...
File src/libplatform/tracing/tracing-controller.cc (right):

https://codereview.chromium.org/2137013006/diff/60001/src/libplatform/tracing...
src/libplatform/tracing/tracing-controller.cc:102:
UpdateCategoryGroupEnabledFlags();
Move this before Flush to disable more traces before flushing.

[fmeawad, 2016-07-15 21:21:59]

https://codereview.chromium.org/2137013006/diff/60001/src/libplatform/tracing...
File src/libplatform/tracing/tracing-controller.cc (right):

https://codereview.chromium.org/2137013006/diff/60001/src/libplatform/tracing...
src/libplatform/tracing/tracing-controller.cc:102:
UpdateCategoryGroupEnabledFlags();
On 2016/07/15 00:19:01, mattloring wrote:
> Move this before Flush to disable more traces before flushing.

Done.

[fmeawad, 2016-07-18 15:27:16]

fmeawad@chromium.org changed reviewers:
+ jochen@chromium.org, yangguo@chromium.org

[fmeawad, 2016-07-18 15:27:17]

Adding reviewers. PTAL

[jochen (gone - plz use gerrit), 2016-07-20 11:03:25]

I see that you need v8 to load the JSON config. Ideally, however, libplatform
would provide functionality to V8 only, and not depend on it.

What if we move the JSON parser to d8?

https://codereview.chromium.org/2137013006/diff/140001/DEPS
File DEPS (right):

https://codereview.chromium.org/2137013006/diff/140001/DEPS#newcode55
DEPS:55: "+libplatform",
why do you need this?

https://codereview.chromium.org/2137013006/diff/140001/include/libplatform/v8...
File include/libplatform/v8-tracing.h (right):

https://codereview.chromium.org/2137013006/diff/140001/include/libplatform/v8...
include/libplatform/v8-tracing.h:11: #include "include/v8.h"
libplatform should not depend on v8.h

https://codereview.chromium.org/2137013006/diff/140001/include/libplatform/v8...
include/libplatform/v8-tracing.h:17: class TraceObject {
please add a ctor

https://codereview.chromium.org/2137013006/diff/140001/include/libplatform/v8...
include/libplatform/v8-tracing.h:65: };
disallow copy/assign?

[Yang, 2016-07-21 11:26:55]

https://codereview.chromium.org/2137013006/diff/140001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2137013006/diff/140001/src/d8.cc#newcode2490
src/d8.cc:2490: if (options.trace_enabled) {
Can we factor out all of this into a new function? Possibly integrate Jochen's
suggestion of doing JSON.parse from there rather than inside
CreateTraceConfigFromJSON.

https://codereview.chromium.org/2137013006/diff/140001/src/d8.cc#newcode2502
src/d8.cc:2502: char* trace_config_str =
Is this string going to be deleted anywhere? It looks like it would leak.

[mattloring, 2016-07-21 17:08:00]

On 2016/07/20 11:03:25, jochen wrote:
> I see that you need v8 to load the JSON config. Ideally, however, libplatform
> would provide functionality to V8 only, and not depend on it.
> 
> What if we move the JSON parser to d8?
> 

We were hoping to use the config parser in Node.js as well rather than
duplicating
the code. Are there any other options that would allow it to remain shared
between
Node.js and d8?

[jochen (gone - plz use gerrit), 2016-07-22 12:23:44]

On 2016/07/21 at 17:08:00, mattloring wrote:
> On 2016/07/20 11:03:25, jochen wrote:
> > I see that you need v8 to load the JSON config. Ideally, however,
libplatform
> > would provide functionality to V8 only, and not depend on it.
> > 
> > What if we move the JSON parser to d8?
> > 
> 
> We were hoping to use the config parser in Node.js as well rather than
duplicating
> the code. Are there any other options that would allow it to remain shared
between
> Node.js and d8?

we could just add it to v8, it's not really that big

[fmeawad, 2016-07-26 09:03:11]

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

[commit-bot: I haz the power, 2016-07-26 09:03:22]

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

[fmeawad, 2016-07-26 09:08:09]

I have addressed most of the comments, concerning the JSON Config Parser, I
found that it is not a must have for the CL to work since we can use the
DefaultConfig (and other embedders can implement their own default Config for
now)

I will work with lpy closely to try to reland the trace config from JSON part
(as mentioned in the description, it was originally contributed by him)

https://codereview.chromium.org/2137013006/diff/140001/DEPS
File DEPS (right):

https://codereview.chromium.org/2137013006/diff/140001/DEPS#newcode55
DEPS:55: "+libplatform",
On 2016/07/20 11:03:25, jochen wrote:
> why do you need this?

Done.

https://codereview.chromium.org/2137013006/diff/140001/include/libplatform/v8...
File include/libplatform/v8-tracing.h (right):

https://codereview.chromium.org/2137013006/diff/140001/include/libplatform/v8...
include/libplatform/v8-tracing.h:11: #include "include/v8.h"
On 2016/07/20 11:03:25, jochen wrote:
> libplatform should not depend on v8.h

Done.

https://codereview.chromium.org/2137013006/diff/140001/include/libplatform/v8...
include/libplatform/v8-tracing.h:17: class TraceObject {
On 2016/07/20 11:03:25, jochen wrote:
> please add a ctor

Done.

https://codereview.chromium.org/2137013006/diff/140001/include/libplatform/v8...
include/libplatform/v8-tracing.h:65: };
On 2016/07/20 11:03:25, jochen wrote:
> disallow copy/assign?

Done.

https://codereview.chromium.org/2137013006/diff/140001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2137013006/diff/140001/src/d8.cc#newcode2490
src/d8.cc:2490: if (options.trace_enabled) {
On 2016/07/21 11:26:55, Yang wrote:
> Can we factor out all of this into a new function? Possibly integrate Jochen's
> suggestion of doing JSON.parse from there rather than inside
> CreateTraceConfigFromJSON.

It is now much simpler as we removed the trace_config check.

https://codereview.chromium.org/2137013006/diff/140001/src/d8.cc#newcode2502
src/d8.cc:2502: char* trace_config_str =
On 2016/07/21 11:26:55, Yang wrote:
> Is this string going to be deleted anywhere? It looks like it would leak.

Removed with the removal of the JSON config for now.

[commit-bot: I haz the power, 2016-07-26 09:25:33]

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

[commit-bot: I haz the power, 2016-07-26 09:25:34]

Dry run: This issue passed the CQ dry run.

[Yang, 2016-07-26 10:57:48]

On 2016/07/26 09:08:09, fmeawad wrote:
> I have addressed most of the comments, concerning the JSON Config Parser, I
> found that it is not a must have for the CL to work since we can use the
> DefaultConfig (and other embedders can implement their own default Config for
> now)
> 
> I will work with lpy closely to try to reland the trace config from JSON part
> (as mentioned in the description, it was originally contributed by him)
> 
> https://codereview.chromium.org/2137013006/diff/140001/DEPS
> File DEPS (right):
> 
> https://codereview.chromium.org/2137013006/diff/140001/DEPS#newcode55
> DEPS:55: "+libplatform",
> On 2016/07/20 11:03:25, jochen wrote:
> > why do you need this?
> 
> Done.
> 
>
https://codereview.chromium.org/2137013006/diff/140001/include/libplatform/v8...
> File include/libplatform/v8-tracing.h (right):
> 
>
https://codereview.chromium.org/2137013006/diff/140001/include/libplatform/v8...
> include/libplatform/v8-tracing.h:11: #include "include/v8.h"
> On 2016/07/20 11:03:25, jochen wrote:
> > libplatform should not depend on v8.h
> 
> Done.
> 
>
https://codereview.chromium.org/2137013006/diff/140001/include/libplatform/v8...
> include/libplatform/v8-tracing.h:17: class TraceObject {
> On 2016/07/20 11:03:25, jochen wrote:
> > please add a ctor
> 
> Done.
> 
>
https://codereview.chromium.org/2137013006/diff/140001/include/libplatform/v8...
> include/libplatform/v8-tracing.h:65: };
> On 2016/07/20 11:03:25, jochen wrote:
> > disallow copy/assign?
> 
> Done.
> 
> https://codereview.chromium.org/2137013006/diff/140001/src/d8.cc
> File src/d8.cc (right):
> 
> https://codereview.chromium.org/2137013006/diff/140001/src/d8.cc#newcode2490
> src/d8.cc:2490: if (options.trace_enabled) {
> On 2016/07/21 11:26:55, Yang wrote:
> > Can we factor out all of this into a new function? Possibly integrate
Jochen's
> > suggestion of doing JSON.parse from there rather than inside
> > CreateTraceConfigFromJSON.
> 
> It is now much simpler as we removed the trace_config check.
> 
> https://codereview.chromium.org/2137013006/diff/140001/src/d8.cc#newcode2502
> src/d8.cc:2502: char* trace_config_str =
> On 2016/07/21 11:26:55, Yang wrote:
> > Is this string going to be deleted anywhere? It looks like it would leak.
> 
> Removed with the removal of the JSON config for now.

lgtm.

[lpy, 2016-07-26 17:36:18]

lgtm

https://codereview.chromium.org/2137013006/diff/160001/src/v8.gyp
File src/v8.gyp (right):

https://codereview.chromium.org/2137013006/diff/160001/src/v8.gyp#newcode2021
src/v8.gyp:2021: 'libplatform/tracing/trace-config.h',
No trace-config.h

[fmeawad, 2016-07-26 17:46:46]

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

[commit-bot: I haz the power, 2016-07-26 17:46:51]

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

[fmeawad, 2016-07-26 17:56:47]

https://codereview.chromium.org/2137013006/diff/160001/src/v8.gyp
File src/v8.gyp (right):

https://codereview.chromium.org/2137013006/diff/160001/src/v8.gyp#newcode2021
src/v8.gyp:2021: 'libplatform/tracing/trace-config.h',
On 2016/07/26 17:36:18, lpy wrote:
> No trace-config.h

Done.

[rskang, 2016-07-26 18:01:47]

https://codereview.chromium.org/2137013006/diff/180001/include/libplatform/v8...
File include/libplatform/v8-tracing.h (right):

https://codereview.chromium.org/2137013006/diff/180001/include/libplatform/v8...
include/libplatform/v8-tracing.h:174: StringList included_categories_;
Is it possible to expose included_categories_ and excluded_categories_ to
public, or through a getter? To allow modification by V8 embedders.

[commit-bot: I haz the power, 2016-07-26 18:10:54]

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

[commit-bot: I haz the power, 2016-07-26 18:10:55]

Dry run: This issue passed the CQ dry run.

[fmeawad, 2016-07-26 20:11:07]

https://codereview.chromium.org/2137013006/diff/180001/include/libplatform/v8...
File include/libplatform/v8-tracing.h (right):

https://codereview.chromium.org/2137013006/diff/180001/include/libplatform/v8...
include/libplatform/v8-tracing.h:174: StringList included_categories_;
On 2016/07/26 18:01:47, rskang wrote:
> Is it possible to expose included_categories_ and excluded_categories_ to
> public, or through a getter? To allow modification by V8 embedders.

I have added setters for them.

[mattloring, 2016-07-26 21:50:59]

On 2016/07/26 20:11:07, fmeawad wrote:
>
https://codereview.chromium.org/2137013006/diff/180001/include/libplatform/v8...
> File include/libplatform/v8-tracing.h (right):
> 
>
https://codereview.chromium.org/2137013006/diff/180001/include/libplatform/v8...
> include/libplatform/v8-tracing.h:174: StringList included_categories_;
> On 2016/07/26 18:01:47, rskang wrote:
> > Is it possible to expose included_categories_ and excluded_categories_ to
> > public, or through a getter? To allow modification by V8 embedders.
> 
> I have added setters for them.

lgtm

[rskang, 2016-07-26 21:51:38]

On 2016/07/26 20:11:07, fmeawad wrote:
>
https://codereview.chromium.org/2137013006/diff/180001/include/libplatform/v8...
> File include/libplatform/v8-tracing.h (right):
> 
>
https://codereview.chromium.org/2137013006/diff/180001/include/libplatform/v8...
> include/libplatform/v8-tracing.h:174: StringList included_categories_;
> On 2016/07/26 18:01:47, rskang wrote:
> > Is it possible to expose included_categories_ and excluded_categories_ to
> > public, or through a getter? To allow modification by V8 embedders.
> 
> I have added setters for them.

lgtm

[fmeawad, 2016-07-26 21:52:25]

The CQ bit was checked by fmeawad@chromium.org

[fmeawad, 2016-07-26 21:52:25]

The patchset sent to the CQ was uploaded after l-g-t-m from lpy@chromium.org,
yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2137013006/#ps200001
(title: "Add Setters for TraceConfig categories")

[commit-bot: I haz the power, 2016-07-26 21:52:34]

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-07-26 22:13:43]

Description was changed from

==========
[Tracing] V8 Tracing Controller

V8 has had a trace event macro interface for while, but without a tracing
controller a standalone V8 would be unable to collect traces.

This CL introduces a complete Tracing Controller system for V8.
It is fully function except that it does not yet store trace event args.

This CL has a few components,
The tracing controller itself, contributed by the author of this CL
The Trace config (including the parser), contributed by lpy@
The Trace Object, Trace Writer, and Trace Buffer are all contributed by rksang@

BUG=v8:4561
LOG=N
==========

to

==========
[Tracing] V8 Tracing Controller

V8 has had a trace event macro interface for while, but without a tracing
controller a standalone V8 would be unable to collect traces.

This CL introduces a complete Tracing Controller system for V8.
It is fully function except that it does not yet store trace event args.

This CL has a few components,
The tracing controller itself, contributed by the author of this CL
The Trace config (including the parser), contributed by lpy@
The Trace Object, Trace Writer, and Trace Buffer are all contributed by rksang@

BUG=v8:4561
LOG=N
==========

[commit-bot: I haz the power, 2016-07-26 22:13:44]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-07-26 22:15:52]

Description was changed from

==========
[Tracing] V8 Tracing Controller

V8 has had a trace event macro interface for while, but without a tracing
controller a standalone V8 would be unable to collect traces.

This CL introduces a complete Tracing Controller system for V8.
It is fully function except that it does not yet store trace event args.

This CL has a few components,
The tracing controller itself, contributed by the author of this CL
The Trace config (including the parser), contributed by lpy@
The Trace Object, Trace Writer, and Trace Buffer are all contributed by rksang@

BUG=v8:4561
LOG=N
==========

to

==========
[Tracing] V8 Tracing Controller

V8 has had a trace event macro interface for while, but without a tracing
controller a standalone V8 would be unable to collect traces.

This CL introduces a complete Tracing Controller system for V8.
It is fully function except that it does not yet store trace event args.

This CL has a few components,
The tracing controller itself, contributed by the author of this CL
The Trace config (including the parser), contributed by lpy@
The Trace Object, Trace Writer, and Trace Buffer are all contributed by rksang@

BUG=v8:4561
LOG=N

Committed: https://crrev.com/3d598452679ce208ad9b2f48e0fb3fae352ce375
Cr-Commit-Position: refs/heads/master@{#38073}
==========

[commit-bot: I haz the power, 2016-07-26 22:15:53]

Patchset 11 (id:??) landed as
https://crrev.com/3d598452679ce208ad9b2f48e0fb3fae352ce375
Cr-Commit-Position: refs/heads/master@{#38073}

[lpy, 2016-07-26 22:39:18]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/2183943002/ by lpy@chromium.org.

The reason for reverting is: Revert this CL due to V8 Arm Builder failure and V8
Mips Builder failure.

https://build.chromium.org/p/client.v8.ports/builders/V8%20Arm%20-%20builder/...

https://build.chromium.org/p/client.v8.ports/builders/V8%20Mips%20-%20builder....