Allow tracing in third_party libraries

Allow tracing in third_party libraries

This is step 1 -- refactor the code to allow us to split up trace_event.h into trace_event.h and trace_event_export.h/cc.

Step 2 is to cut out trace_event_export.h/cc.

The _export.h/cc will be laid out so they can be copied to other third_party libraries and then tweaked to call through to chromium's internal tracing API via platform APIs or function pointers.

To make these APIs easily exportable, this change primarily aims to eliminate the custom objects (ie: TraceValue) and types that will cause problems when defined in multiple libraries. The macros and internal namespace are destined for trace_event_export.h/cc in a future CL.

BUG=109779
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=117598

[jbates, 2012-01-10 22:15:34]

nduca: please review
jar: base/ OWNERS
phajdan: base/test/ OWNERS
darin: webkit/ OWNERS

[darin (slow to review), 2012-01-10 23:29:21]

LGTM for webkit/

[Paweł Hajdan Jr., 2012-01-11 07:27:24]

base/test OWNER LGTM (but please do a detailed review, I only took a brief look)

[Tom Hudson, 2012-01-11 13:24:10]

I'm not sure I understand the need for this? We didn't have any trouble linking
Skia into Chromium's tracing framework.

Skia has #define SK_TRACE_EVENT0/1/2 analogous to those in Chromium.
If they aren't defined by user-provided configuration files or gyp files, they
default to empty; we provide a sample implementation that routes them to stderr.
src/skia/config/SkUserConfig.h can set SK_TRACE_EVENTn up to redirect to
TRACE_EVENTn.

[jbates, 2012-01-11 18:09:40]

On 2012/01/11 13:24:10, Tom Hudson wrote:
> I'm not sure I understand the need for this? We didn't have any trouble
linking
> Skia into Chromium's tracing framework.

Looks pretty good, and that may be enough for Skia. Chromium tracing has a few
extra bells and whistles that we would like to get access to from third_party
libraries. To name a few:
- low-overhead disabled state (sufficient for tracing in official builds)
- categories (separate enable flag per category for filtering results)
- support for multiple value types (not just string)

Some of these features are needed by WebKit and ANGLE. Since we're covering
those, may as well wire through to Skia also unless there are other issues that
I'm overlooking.

> 
> Skia has #define SK_TRACE_EVENT0/1/2 analogous to those in Chromium.
> If they aren't defined by user-provided configuration files or gyp files, they
> default to empty; we provide a sample implementation that routes them to
stderr.
> src/skia/config/SkUserConfig.h can set SK_TRACE_EVENTn up to redirect to
> TRACE_EVENTn.

Excellent, looks like this is where I would wire in the new features as well?

[nduca, 2012-01-11 21:49:11]

Looking awesome!

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:164: /// Customize these defines to call through to
platform tracing APIs.
You know, I'm starting to think we should make trace_event.h be a stub code that
#defines the TRACE_EVENT_API macros
#includes trace_event_macros.h
#includes trace_event_impl.h

Sorry it took me this long to figure out what you meant.

This would allow you to put nice documentation on the trace_event_macros.h file
that explains what it does. Something like "This header is designed to be give
you trace_event macros without specifying how the events actually get collected
and stored. If you need to expose trace event to some other universe, you can
copy-and-paste this file and just implement API* macros."

I'm not sure the TRACE_EVENT_NAMESPACE adds a lot of value here. That seems
specific to the binding of the macros to the base implementation, no? That is,
webkit wont even need that, right? There, we'll just point the macros at
something completely whacko.

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:598: // New types can be added, but existing values
must never change.
Might want to clarify them 'must never change'. I think you mean something like
"downstream versions of trace_event reliy on these, so if you change them, you
need to very carefully update downstream as well."

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:654: const char* as_string;
No clue what goog style says, so I'll assume as_x is what le-style-gods-decree?

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:690: // 64-bit types:
not sure we need these comments, but if you feel they're value adds, then at
least make them stentences. // These are all 64 bit types.

[nduca, 2012-01-11 21:55:57]

For webkit, we definitely need this. There, we can't include base/ directly.

[jbates, 2012-01-11 22:44:35]

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:164: /// Customize these defines to call through to
platform tracing APIs.
On 2012/01/11 21:49:11, nduca wrote:
> You know, I'm starting to think we should make trace_event.h be a stub code
that
> #defines the TRACE_EVENT_API macros
> #includes trace_event_macros.h
> #includes trace_event_impl.h

I think this sounds good, but this is a step-2 feedback, right? I'm deferring
anything related to splitting up files to a follow-on CL so that we can clearly
see the refactoring changes first.

> 
> Sorry it took me this long to figure out what you meant.
> 
> This would allow you to put nice documentation on the trace_event_macros.h
file
> that explains what it does. Something like "This header is designed to be give
> you trace_event macros without specifying how the events actually get
collected
> and stored. If you need to expose trace event to some other universe, you can
> copy-and-paste this file and just implement API* macros."
> 
> I'm not sure the TRACE_EVENT_NAMESPACE adds a lot of value here. That seems
> specific to the binding of the macros to the base implementation, no? That is,
> webkit wont even need that, right? There, we'll just point the macros at
> something completely whacko.

There are some local types that get copied to other libraries such as
TraceStringWithCopy, which undoubtedly would be put inside a namespace in
third_party libraries. To reduce the hand-editing required to fix up the _export
header, I tried to put everything that will need hand-editing in this section.
Alternatively I could just choose a namespace like "trace_event_internal::" and
throw all this stuff in there -- come to think of it that is a lot cleaner.
Sound good?

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:598: // New types can be added, but existing values
must never change.
On 2012/01/11 21:49:11, nduca wrote:
> Might want to clarify them 'must never change'. I think you mean something
like
> "downstream versions of trace_event reliy on these, so if you change them, you
> need to very carefully update downstream as well."

I guess I don't see any reason to discuss changing existing values, because if
we've designed this API properly, changing values should never be necessary
(analogous to OpenGL values). Is there a less confusing way to write "existing
values must never change"?

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:654: const char* as_string;
On 2012/01/11 21:49:11, nduca wrote:
> No clue what goog style says, so I'll assume as_x is what
le-style-gods-decree?

The TraceValue union was defined this way already, but let me know if there's a
style problem.

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:690: // 64-bit types:
On 2012/01/11 21:49:11, nduca wrote:
> not sure we need these comments, but if you feel they're value adds, then at
> least make them stentences. // These are all 64 bit types.

Does seem redundant with the first comment -- will remove them.

[jbates, 2012-01-11 23:52:44]

nduca, jar: ptal

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:164: /// Customize these defines to call through to
platform tracing APIs.
On 2012/01/11 22:44:35, jbates wrote:
> On 2012/01/11 21:49:11, nduca wrote:
> > You know, I'm starting to think we should make trace_event.h be a stub code
> that
> > #defines the TRACE_EVENT_API macros
> > #includes trace_event_macros.h
> > #includes trace_event_impl.h
> 
> I think this sounds good, but this is a step-2 feedback, right? I'm deferring
> anything related to splitting up files to a follow-on CL so that we can
clearly
> see the refactoring changes first.
> 
> > 
> > Sorry it took me this long to figure out what you meant.
> > 
> > This would allow you to put nice documentation on the trace_event_macros.h
> file
> > that explains what it does. Something like "This header is designed to be
give
> > you trace_event macros without specifying how the events actually get
> collected
> > and stored. If you need to expose trace event to some other universe, you
can
> > copy-and-paste this file and just implement API* macros."
> > 
> > I'm not sure the TRACE_EVENT_NAMESPACE adds a lot of value here. That seems
> > specific to the binding of the macros to the base implementation, no? That
is,
> > webkit wont even need that, right? There, we'll just point the macros at
> > something completely whacko.
> 
> There are some local types that get copied to other libraries such as
> TraceStringWithCopy, which undoubtedly would be put inside a namespace in
> third_party libraries. To reduce the hand-editing required to fix up the
_export
> header, I tried to put everything that will need hand-editing in this section.
> Alternatively I could just choose a namespace like "trace_event_internal::"
and
> throw all this stuff in there -- come to think of it that is a lot cleaner.
> Sound good?

Removed TRACE_EVENT_NAMESPACE.

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:598: // New types can be added, but existing values
must never change.
On 2012/01/11 21:49:11, nduca wrote:
> Might want to clarify them 'must never change'. I think you mean something
like
> "downstream versions of trace_event reliy on these, so if you change them, you
> need to very carefully update downstream as well."

Done. Ignore my previous comment, I was confused about whether we were talking
about chromium versions of this comment or third_party versions.

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:690: // 64-bit types:
On 2012/01/11 22:44:35, jbates wrote:
> On 2012/01/11 21:49:11, nduca wrote:
> > not sure we need these comments, but if you feel they're value adds, then at
> > least make them stentences. // These are all 64 bit types.
> 
> Does seem redundant with the first comment -- will remove them.

Done.

[jar (doing other things), 2012-01-12 00:15:36]

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:166: #define TRACE_EVENT_NAMESPACE base::debug
What does this macro help you with?

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:509: static const volatile bool* \
What does the use of volatile do for you?  (for most users, not doing hardware
register coding, this keyword is misused).

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:519: #define INTERNAL_TRACE_EVENT_ADD(phase, category,
name, flags, ...) \
Is ellipsis (vararg) defined for the preprocessor???  I thought this was only
just part of the emerging standard... and I'm a little surprised to see it used
(as it may make porting harder).  

Am I too old school?

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:524: ##__VA_ARGS__); \
... and you're also using the ## gcc compatibility hack.

Again, for portability, I'm surprised we're doing this stuff.  Can you give me
any pointers?

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event_unit...
File base/debug/trace_event_unittest.cc (right):

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:4: 
You added a lot of macros.  Shouldn't there be tests for them?

http://codereview.chromium.org/9155024/diff/14001/base/test/trace_event_analy...
File base/test/trace_event_analyzer.cc (right):

http://codereview.chromium.org/9155024/diff/14001/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:448: {
nit: open curly should come at the end of line 447.
Close curly (such as on line 454) should match the indent of the "c" in "case."

To put it another way: When you use curlies, the indent of the body of teh case
does not change (it is still just two deeper than the "c" in "case."

http://codereview.chromium.org/9155024/diff/14001/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:518: {
nit: indent on curlies throughout this file in case statements.

[jbates, 2012-01-12 00:52:20]

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:166: #define TRACE_EVENT_NAMESPACE base::debug
On 2012/01/12 00:15:36, jar wrote:
> What does this macro help you with?

Done. (removed)

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:509: static const volatile bool* \
On 2012/01/12 00:15:36, jar wrote:
> What does the use of volatile do for you?  (for most users, not doing hardware
> register coding, this keyword is misused).

It's useful in this case, because it tells the compiler that the load of this
bool can't be moved from the inside to the outside of a loop.

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:519: #define INTERNAL_TRACE_EVENT_ADD(phase, category,
name, flags, ...) \
On 2012/01/12 00:15:36, jar wrote:
> Is ellipsis (vararg) defined for the preprocessor???  I thought this was only
> just part of the emerging standard... and I'm a little surprised to see it
used
> (as it may make porting harder).  
> 
> Am I too old school?

It's used in base/logging.h so I'm hoping I can use it here to avoid a lot of
macro code duplication.

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:524: ##__VA_ARGS__); \
On 2012/01/12 00:15:36, jar wrote:
> ... and you're also using the ## gcc compatibility hack.
> 
> Again, for portability, I'm surprised we're doing this stuff.  Can you give me
> any pointers?

I just stole this from base/logging.h, which did not appear to be a platform
dependent implementation (surprising to me as well).

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event_unit...
File base/debug/trace_event_unittest.cc (right):

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:4: 
On 2012/01/12 00:15:36, jar wrote:
> You added a lot of macros.  Shouldn't there be tests for them?

Done.

http://codereview.chromium.org/9155024/diff/14001/base/test/trace_event_analy...
File base/test/trace_event_analyzer.cc (right):

http://codereview.chromium.org/9155024/diff/14001/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:448: {
On 2012/01/12 00:15:36, jar wrote:
> nit: open curly should come at the end of line 447.
> Close curly (such as on line 454) should match the indent of the "c" in
"case."
> 
> To put it another way: When you use curlies, the indent of the body of teh
case
> does not change (it is still just two deeper than the "c" in "case."

Done.

http://codereview.chromium.org/9155024/diff/14001/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:518: {
On 2012/01/12 00:15:36, jar wrote:
> nit: indent on curlies throughout this file in case statements.

Done.

[nduca, 2012-01-12 01:41:32]

On 2012/01/11 23:52:44, jbates wrote:
> > I think this sounds good, but this is a step-2 feedback, right? I'm
deferring
> > anything related to splitting up files to a follow-on CL so that we can
> clearly
> > see the refactoring changes first.

At minimum, you can get the documentation text right. I think you might be
better off just doing the move now. "Phase 2" usually means "never," even with
your changes. C.f. some of the stuff we said we'd do with TraceAnalyzer, whose
design we haven't substantially modified since we began.


> > There are some local types that get copied to other libraries such as
> > TraceStringWithCopy, which undoubtedly would be put inside a namespace in
> > third_party libraries. To reduce the hand-editing required to fix up the
> _export
> > header, I tried to put everything that will need hand-editing in this
section.
> > Alternatively I could just choose a namespace like "trace_event_internal::"

WebKit doesn't really like you putting down namespaces. They have WebCore, WTF
and WebKit. That's all they have, iirw.

[nduca, 2012-01-12 01:44:17]

LGTM with good doc strings

[jar (doing other things), 2012-01-12 02:20:03]

The big comment is about volatile.  I really suspect this is not working as
intended.

There are also a few nits.  Perhaps the biggest (unstated) meta-nit is that
these macros and functions often have a LOT of args.  Read this stuff is pretty
nearly impossible ('cause I can't remember a long list of positions-to-name
declarations).  I'm praying that argument mismatches in types will catch errors
that I can't possible see :-(.  I sure wish it could be simplified in many of
these calls.  Have you thought about any way the code could be made more
readable in this area?

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:509: static const volatile bool* \
I don't think volatile has such ordering semantics.  Can you give me a pointer
to where this is stated? 

On 2012/01/12 00:52:20, jbates wrote:
> On 2012/01/12 00:15:36, jar wrote:
> > What does the use of volatile do for you?  (for most users, not doing
hardware
> > register coding, this keyword is misused).
> 
> It's useful in this case, because it tells the compiler that the load of this
> bool can't be moved from the inside to the outside of a loop.

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:519: #define INTERNAL_TRACE_EVENT_ADD(phase, category,
name, flags, ...) \
Interesting.  OK.  Thanks for the example.  It is still surprising to me.

On 2012/01/12 00:52:20, jbates wrote:
> On 2012/01/12 00:15:36, jar wrote:
> > Is ellipsis (vararg) defined for the preprocessor???  I thought this was
only
> > just part of the emerging standard... and I'm a little surprised to see it
> used
> > (as it may make porting harder).  
> > 
> > Am I too old school?
> 
> It's used in base/logging.h so I'm hoping I can use it here to avoid a lot of
> macro code duplication.

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:936: #define
INTERNAL_DECLARE_SET_TRACE_VALUE(actual_type, \
I'm not a template master at all... but couldn't this be done better with
templates, and avoid macros??  I don't see the magic that macros bring to the
table here.

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:964: 
I you really want to use a macro... you may as well undef it here.

http://codereview.chromium.org/9155024/diff/21002/base/debug/trace_event.cc
File base/debug/trace_event.cc (right):

http://codereview.chromium.org/9155024/diff/21002/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:58: volatile bool
g_category_enabled[TRACE_EVENT_MAX_CATEGORIES] = { false };
Here again, I doubt volatile is providing significant semantics.

This is probably why you're passing it around in various arguments and macros...
but I'd like to see the reference which suggests this is much more than a no-op
(or a restriction on how many times the vector is accessed, in reads and writes,
to match requested accesses).

It would be very helpful if you would explain what you think it precludes
specifically.  What do you think could happen (at the compile phase) that would
"break" if you didn't have this?

http://codereview.chromium.org/9155024/diff/21002/base/test/trace_event_analy...
File base/test/trace_event_analyzer.cc (right):

http://codereview.chromium.org/9155024/diff/21002/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:271: } else {
nit: with all the returns, you don't need an else clause.

http://codereview.chromium.org/9155024/diff/21002/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:474: return false;
nit: I don't know that it is worth changing... but I always try to have the
"simple" case as the predicate in the if, so that the complex one doesn't need
to be indented.  In addition, you get away with not having curlies (around a
solo statement, such as "return false.".

Sometimes you abide by this (early return) form, but sometimes not.  To me, it
always means less code to look at.

[joth, 2012-01-12 16:14:46]

just a drive-by...

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:509: static const volatile bool* \
On 2012/01/12 02:20:03, jar wrote:
> I don't think volatile has such ordering semantics.  Can you give me a pointer
> to where this is stated? 
> 

Not a normative reference, but very informative:
http://kerneltrap.org/Linux/Volatile_Superstition


"""
Volatile doesn't mean it can't be reordered; volatile means the
accesses can't be eliminated.
...

So if "volatile" makes a difference, it is invariably a sign of a bug in 
serialization (the one exception is for IO - we use "volatile" to avoid 
having to use inline asm for IO on x86) - and for "random values" like 
jiffies).
"""

Not eliminating the memory access is key: I think these bools actually do fall
into this "random values" corner case, where reording is fairly harmless, but
you don't want any accesses to be eliminated.
i.e. absolutely nothing 'bad' would occur w.r.t the production code if this code
happens to read the 'wrong' value on every iteration. The goal here is to stop
the compiler eliminating the inner-loop memory accesses. (whether that's
actually a good trade off though I can't call. Linus's comments about volatile
resulting in crap code generation, which will be a cost in *all* the other
non-loop call-sites!, maybe more important than having the category 'enablement'
be observed by any code already in its loop?)

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:661: const volatile bool* category_enabled,
I think you can achieve your goal by just putting volatile on the pointer
through which the bool accessed (up on line 509).
i.e. No need to permeate it throughout the file. (with reference to the thread I
linked above, this is equivalent to hiding its use inside atomic_read() rather
than putting it on the typedef itself)

[jbates, 2012-01-12 19:41:29]

Regarding the macro spam, I do have an idea: we can cut out about 2/3s of the
macros by using variadic macros and replace TRACE_EVENT0/1/2 with just
TRACE_EVENT. This touches a lot of files to do though, so if I can get buy-in I
will do it as a followup CL to avoid reviewer confusion.

I mention stuff about strict-aliasing below in one of my comments, but that's
somewhat moot anyway because we don't enable it in our builds. (Side note: Why
don't we enable strict-aliasing? It definitely affects performance in opt
builds. Sure, it's easy to write code that breaks strict aliasing rules, but we
should not let that get past reviewers and our unit tests should catch those
bugs, right? :)

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:661: const volatile bool* category_enabled,
On 2012/01/12 16:14:46, joth wrote:
> I think you can achieve your goal by just putting volatile on the pointer
> through which the bool accessed (up on line 509).
> i.e. No need to permeate it throughout the file. (with reference to the thread
I
> linked above, this is equivalent to hiding its use inside atomic_read() rather
> than putting it on the typedef itself)

Just getting rid of volatile completely, as it doesn't seem worth the perf
tradeoff as you mentioned.

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:936: #define
INTERNAL_DECLARE_SET_TRACE_VALUE(actual_type, \
On 2012/01/12 02:20:03, jar wrote:
> I'm not a template master at all... but couldn't this be done better with
> templates, and avoid macros??  I don't see the magic that macros bring to the
> table here.

In this case we need full control over the supported types, so templates would
be too flexible. I did try to get rid of the macro anyway, but it results in a
lot of code duplication, so the macros seems to be the lesser of two evils. I'm
fine either way though, let me know if you'd prefer the functions typed out.

http://codereview.chromium.org/9155024/diff/14001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:964: 
On 2012/01/12 02:20:03, jar wrote:
> I you really want to use a macro... you may as well undef it here.

Done.

http://codereview.chromium.org/9155024/diff/21002/base/debug/trace_event.cc
File base/debug/trace_event.cc (right):

http://codereview.chromium.org/9155024/diff/21002/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:58: volatile bool
g_category_enabled[TRACE_EVENT_MAX_CATEGORIES] = { false };
On 2012/01/12 02:20:03, jar wrote:
> Here again, I doubt volatile is providing significant semantics.
> 
> This is probably why you're passing it around in various arguments and
macros...
> but I'd like to see the reference which suggests this is much more than a
no-op
> (or a restriction on how many times the vector is accessed, in reads and
writes,
> to match requested accesses).
> 
> It would be very helpful if you would explain what you think it precludes
> specifically.  What do you think could happen (at the compile phase) that
would
> "break" if you didn't have this?

The only behavior I expected from volatile was to disable compiler optimizations
on these bools in the macros, which I believe is what volatile does. For
example, if someone were to write this code:
for (;;) {
  TRACE_EVENT0("category", "yippee!");
}
Without volatile, the compiler might be allowed to optimize the load of the bool
outside this loop, which means the state of tracing would either be permanently
on or off depending on the initial value of the bool. If another thread were to
change the enabled state, this loop would continue on with an invalid enabled
state.

However, I think I was being too pedantic, because this is not a tracing use
case that we want to support.

I'm also humbled to learn that MSVC goes above and beyond how most compilers
treat volatile: MSVC applies Acquire and Release semantics to volatile reads and
writes, respectively. We really don't want that kind of overhead in this code.

I've changed the type from volatile bool to unsigned char. Moving away from bool
allows the API to be used from C code, but it also has the nice side-effect that
char types can't be optimized as much because they are allowed to alias any
other type by strict-aliasing rules. It's also nice because even if this char is
garbage memory (because of threaded race conditions as we've discussed in the
past), it will always evaluate to true or false. The bool is probably the same
in practice, but in theory it may be undefined behavior when evaluating a bool
whose bits are garbage.

http://codereview.chromium.org/9155024/diff/21002/base/test/trace_event_analy...
File base/test/trace_event_analyzer.cc (right):

http://codereview.chromium.org/9155024/diff/21002/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:271: } else {
On 2012/01/12 02:20:03, jar wrote:
> nit: with all the returns, you don't need an else clause.

Done.

http://codereview.chromium.org/9155024/diff/21002/base/test/trace_event_analy...
base/test/trace_event_analyzer.cc:474: return false;
On 2012/01/12 02:20:03, jar wrote:
> nit: I don't know that it is worth changing... but I always try to have the
> "simple" case as the predicate in the if, so that the complex one doesn't need
> to be indented.  In addition, you get away with not having curlies (around a
> solo statement, such as "return false.".
> 
> Sometimes you abide by this (early return) form, but sometimes not.  To me, it
> always means less code to look at.

Done.

[nduca, 2012-01-12 19:52:42]

On 2012/01/12 19:41:29, jbates wrote:
> Regarding the macro spam, I do have an idea: we can cut out about 2/3s of the
> macros by using variadic macros and replace TRACE_EVENT0/1/2 with just
> TRACE_EVENT. This touches a lot of files to do though, so if I can get buy-in
I
> will do it as a followup CL to avoid reviewer confusion.

I'd prefer we minimize reliance on vararg macros and anthing else that limits
portability. While assumptions of compiler revs and in particular newer visual
studios is fine for the chrome ecosystem, some of our upstream ecosystems have
no such assumption, which would in turn limit one of the primary goals here,
which is to have something we can copy-and-paste into upstreams with minimal
revision.

[Tom Hudson, 2012-01-12 20:18:12]

> I mention stuff about strict-aliasing below in one of my comments, but that's
> somewhat moot anyway because we don't enable it in our builds. (Side note: Why
> don't we enable strict-aliasing? It definitely affects performance in opt
> builds. Sure, it's easy to write code that breaks strict aliasing rules, but
we
> should not let that get past reviewers and our unit tests should catch those
> bugs, right? :)

Tangent: Skia breaks strict-aliasing; we can't figure out how to fix it; and the
performance improvement we get from it seems to be (1) minimal and (2)
inexplicable. Any ideas reviewers here have would be welcome at
http://codereview.appspot.com/5515047/

[jar (doing other things), 2012-01-12 21:48:43]

http://codereview.chromium.org/9155024/diff/21002/base/debug/trace_event.cc
File base/debug/trace_event.cc (right):

http://codereview.chromium.org/9155024/diff/21002/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:58: volatile bool
g_category_enabled[TRACE_EVENT_MAX_CATEGORIES] = { false };
Two things:

a) You're movin' away from volatile, so this discussion becomes less important.

b) As you noted, volatile doesn't imply any memory barriers, even though you say
MSVC seem to put them in.  As a result, even if you convinced the compiler not
to hoist the access out of the loop, the CPU, with its cache, could effectively
do so!! Hence the data "could" be loaded from main memory once (into cache), and
then repeatedly read (via compiled code) from that cache, effectively hoisting
the access "out of the loop."  If you think that result is problematic, perhaps
you should be using barriers.... but I suspect it is not needed.

On 2012/01/12 19:41:29, jbates wrote:
> On 2012/01/12 02:20:03, jar wrote:
> > Here again, I doubt volatile is providing significant semantics.
> > 
> > This is probably why you're passing it around in various arguments and
> macros...
> > but I'd like to see the reference which suggests this is much more than a
> no-op
> > (or a restriction on how many times the vector is accessed, in reads and
> writes,
> > to match requested accesses).
> > 
> > It would be very helpful if you would explain what you think it precludes
> > specifically.  What do you think could happen (at the compile phase) that
> would
> > "break" if you didn't have this?
> 
> The only behavior I expected from volatile was to disable compiler
optimizations
> on these bools in the macros, which I believe is what volatile does. For
> example, if someone were to write this code:
> for (;;) {
>   TRACE_EVENT0("category", "yippee!");
> }
> Without volatile, the compiler might be allowed to optimize the load of the
bool
> outside this loop, which means the state of tracing would either be
permanently
> on or off depending on the initial value of the bool. If another thread were
to
> change the enabled state, this loop would continue on with an invalid enabled
> state.
> 
> However, I think I was being too pedantic, because this is not a tracing use
> case that we want to support.
> 
> I'm also humbled to learn that MSVC goes above and beyond how most compilers
> treat volatile: MSVC applies Acquire and Release semantics to volatile reads
and
> writes, respectively. We really don't want that kind of overhead in this code.
> 
> I've changed the type from volatile bool to unsigned char. Moving away from
bool
> allows the API to be used from C code, but it also has the nice side-effect
that
> char types can't be optimized as much because they are allowed to alias any
> other type by strict-aliasing rules. It's also nice because even if this char
is
> garbage memory (because of threaded race conditions as we've discussed in the
> past), it will always evaluate to true or false. The bool is probably the same
> in practice, but in theory it may be undefined behavior when evaluating a bool
> whose bits are garbage.

http://codereview.chromium.org/9155024/diff/28001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

http://codereview.chromium.org/9155024/diff/28001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:178: //                    const unsigned char*
category_enabled,
nit: IMO, bool is a better type. It adds to readability. I don't believe you
need to fear how it is evaluated if it is garbage.

http://codereview.chromium.org/9155024/diff/28001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:817: long long threshold,
nit: Style guide argues for types like int64.  I'm not sure why you changed
these.

I'd suggest sticking with the well defined (sized) types that google3 (or our
base types) define.

[jbates, 2012-01-12 22:22:11]

http://codereview.chromium.org/9155024/diff/21002/base/debug/trace_event.cc
File base/debug/trace_event.cc (right):

http://codereview.chromium.org/9155024/diff/21002/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:58: volatile bool
g_category_enabled[TRACE_EVENT_MAX_CATEGORIES] = { false };
On 2012/01/12 21:48:43, jar wrote:
> Two things:
> 
> a) You're movin' away from volatile, so this discussion becomes less
important.
> 
> b) As you noted, volatile doesn't imply any memory barriers, even though you
say
> MSVC seem to put them in.  As a result, even if you convinced the compiler not
> to hoist the access out of the loop, the CPU, with its cache, could
effectively
> do so!! Hence the data "could" be loaded from main memory once (into cache),
and
> then repeatedly read (via compiled code) from that cache, effectively hoisting
> the access "out of the loop." 

I don't think that is possible thanks to cache coherency in x86 and most modern
CPUs. In x86, it's the MESI protocol that provides cache coherency
(http://en.wikipedia.org/wiki/MESI_protocol). With that, it's not possible for
load/store instructions to the same address to continue to return different
results from different caches. From the article:

"A cache may satisfy a read from any state except Invalid. An Invalid line must
be fetched (to the Shared or Exclusive states) to satisfy a read."

"A write may only be performed if the cache line is in the Modified or Exclusive
state. If it is in the Shared state, all other cached copies must be invalidated
first. This is typically done by a broadcast operation known as Request For
Ownership (RFO)."

> If you think that result is problematic, perhaps
> you should be using barriers.... but I suspect it is not needed.
> 
> On 2012/01/12 19:41:29, jbates wrote:
> > On 2012/01/12 02:20:03, jar wrote:
> > > Here again, I doubt volatile is providing significant semantics.
> > > 
> > > This is probably why you're passing it around in various arguments and
> > macros...
> > > but I'd like to see the reference which suggests this is much more than a
> > no-op
> > > (or a restriction on how many times the vector is accessed, in reads and
> > writes,
> > > to match requested accesses).
> > > 
> > > It would be very helpful if you would explain what you think it precludes
> > > specifically.  What do you think could happen (at the compile phase) that
> > would
> > > "break" if you didn't have this?
> > 
> > The only behavior I expected from volatile was to disable compiler
> optimizations
> > on these bools in the macros, which I believe is what volatile does. For
> > example, if someone were to write this code:
> > for (;;) {
> >   TRACE_EVENT0("category", "yippee!");
> > }
> > Without volatile, the compiler might be allowed to optimize the load of the
> bool
> > outside this loop, which means the state of tracing would either be
> permanently
> > on or off depending on the initial value of the bool. If another thread were
> to
> > change the enabled state, this loop would continue on with an invalid
enabled
> > state.
> > 
> > However, I think I was being too pedantic, because this is not a tracing use
> > case that we want to support.
> > 
> > I'm also humbled to learn that MSVC goes above and beyond how most compilers
> > treat volatile: MSVC applies Acquire and Release semantics to volatile reads
> and
> > writes, respectively. We really don't want that kind of overhead in this
code.
> > 
> > I've changed the type from volatile bool to unsigned char. Moving away from
> bool
> > allows the API to be used from C code, but it also has the nice side-effect
> that
> > char types can't be optimized as much because they are allowed to alias any
> > other type by strict-aliasing rules. It's also nice because even if this
char
> is
> > garbage memory (because of threaded race conditions as we've discussed in
the
> > past), it will always evaluate to true or false. The bool is probably the
same
> > in practice, but in theory it may be undefined behavior when evaluating a
bool
> > whose bits are garbage.
>

http://codereview.chromium.org/9155024/diff/28001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

http://codereview.chromium.org/9155024/diff/28001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:178: //                    const unsigned char*
category_enabled,
On 2012/01/12 21:48:43, jar wrote:
> nit: IMO, bool is a better type. It adds to readability. I don't believe you
> need to fear how it is evaluated if it is garbage.

The main reasoning for not using bool in this API is that it allows the tracing
API to be exported to C code. I don't think we have any C libraries where we
need tracing, but it seems a small enough price to pay for compatibility?

http://codereview.chromium.org/9155024/diff/28001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:817: long long threshold,
On 2012/01/12 21:48:43, jar wrote:
> nit: Style guide argues for types like int64.  I'm not sure why you changed
> these.
> 
> I'd suggest sticking with the well defined (sized) types that google3 (or our
> base types) define.

In order to make this API available to third party libraries, we can't depend on
the int64 type family because it's only valid in chromium. I believe this is
done for other chromium APIs as well where they are called via WebKit, for
example.

[jar (doing other things), 2012-01-12 23:41:55]

IMO, re-usability in C context seems like a weaker reason to avoid style guide
suggestions for things like int64 etc.

I can vaguely buy into better utility with third party sub-modules, such as
webkit.... but I'm not truly convinced we should avoid our typedefs (I'd rather
see us include our base type headers, and have the 3rd party never notice).

I think the code is too valuable to delay further with those concerns (and I
think it offers great utility) so LGTM... but please be sure you need to go this
route as you proceed.

[commit-bot: I haz the power, 2012-01-12 23:43:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbates@chromium.org/9155024/18013

[commit-bot: I haz the power, 2012-01-13 00:01:05]

Try job failure for 9155024-18013 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-01-13 00:41:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbates@chromium.org/9155024/10018

[commit-bot: I haz the power, 2012-01-13 03:12:48]

Change committed as 117598