Merge gpu_trace_event back into base/debug/trace_event

Merge gpu_trace_event back into base/debug/trace_event. 
Initial land attempt at http://codereview.chromium.org/6551019/ 
gpu_trace_event fork at http://codereview.chromium.org/6691013

- gpu_trace_event renamed to base/debug/trace_event and modified as appropriate for base::debug
- Unit Tests implemented for trace_event
- gpu_common library removed (was added only for gpu_trace_event)
- Existing calls to trace_event suffixed with _ETW to make ETW calls (see decisions and incremental plans at end of 6551019)
- GPU trace calls renamed to new calls. 
- Tracing switch removed from test_shell, as linux log file support removed.

BUG=79509
TEST=trace_event_win_unittest and about:gpu

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

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

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

[scheib, 2011-04-15 01:15:11]

Let's see if we can merge gpu_trace_event back into base/debug/trace_event.

[scheib, 2011-04-15 01:16:50]

Trybots: they're failing since I was on TOT and the trybots running LKGR which
hasn't caught up yet. I'll re-kick these later.

[Sigurður Ásgeirsson, 2011-04-15 14:24:55]

I'd like to rehash some of my comments from a previous incarnation of this CL:

I have no objection to this change in principle, and it seems like a good idea
to allow in-browser viewing of the trace data. From experience with collating
trace data, I think you might be better off with something more unique than
string to match begin/end pairs. The original idea with the id parameter is to
allow passing in a per-process unique value, like e.g. a this pointer.

Be mindful in implementation and testing of a general base mechanism, that there
are base users who don't tolerate leaking objects - Chrome Frame in particular.

Since this change touches widely, has hairy-*** threading behavior and does IPC,
I'd expect that you'd need some unittests at minimum.

I'll be more than happy to do a detailed review of changes to the windows
implementation if you decide to go forward, but I don't currently have the time
to commit to reviewing a change of this scope in detail.

- detailed review of the windows specific changes to follow, but in the meantime
I'd suggest adding serious unittesting for the new implementation.

[scheib, 2011-04-15 15:29:32]

On 2011/04/15 14:24:55, Ruðrugis wrote:
> I'd like to rehash some of my comments from a previous incarnation of this CL:
> 
> I have no objection to this change in principle, and it seems like a good idea
> to allow in-browser viewing of the trace data. From experience with collating
> trace data, I think you might be better off with something more unique than
> string to match begin/end pairs. The original idea with the id parameter is to
> allow passing in a per-process unique value, like e.g. a this pointer.

That was a good idea, and this change incorporates it. The end events take
parameters that a parser can use to match unique pairs across processes or in
disjoint sets. 

> Be mindful in implementation and testing of a general base mechanism, that
there
> are base users who don't tolerate leaking objects - Chrome Frame in
particular.

Can you be concrete here? The code cleans up and isn't making any assumptions
about leaking.

> Since this change touches widely, has hairy-*** threading behavior and does
IPC,
> I'd expect that you'd need some unittests at minimum.

Functional testing is a great idea - and is being discussed (nduca, in relation
to scroll).

> - detailed review of the windows specific changes to follow, but in the
meantime
> I'd suggest adding serious unittesting for the new implementation.

Thx

[Sigurður Ásgeirsson, 2011-04-15 15:35:06]

On Fri, Apr 15, 2011 at 11:29 AM, <scheib@chromium.org> wrote:

> On 2011/04/15 14:24:55, Ruðrugis wrote:
>
>> I'd like to rehash some of my comments from a previous incarnation of this
>> CL:
>>
>
>  I have no objection to this change in principle, and it seems like a good
>> idea
>> to allow in-browser viewing of the trace data. From experience with
>> collating
>> trace data, I think you might be better off with something more unique
>> than
>> string to match begin/end pairs. The original idea with the id parameter
>> is to
>> allow passing in a per-process unique value, like e.g. a this pointer.
>>
>
> That was a good idea, and this change incorporates it. The end events take
> parameters that a parser can use to match unique pairs across processes or
> in
> disjoint sets.
>
>
Awesome.


>
>  Be mindful in implementation and testing of a general base mechanism, that
>>
> there
>
>> are base users who don't tolerate leaking objects - Chrome Frame in
>>
> particular.
>
> Can you be concrete here? The code cleans up and isn't making any
> assumptions
> about leaking.
>
>
This is just a general comment as I haven't reviewed in detail, but it's not
unusual for low-level logging and tracing machinery have awkward
teardown/shutdown behavior.


>
>  Since this change touches widely, has hairy-*** threading behavior and
>> does
>>
> IPC,
>
>> I'd expect that you'd need some unittests at minimum.
>>
>
> Functional testing is a great idea - and is being discussed (nduca, in
> relation
> to scroll).
>
> I'd suggest a unittest as the first order of business.


>
>  - detailed review of the windows specific changes to follow, but in the
>>
> meantime
>
>> I'd suggest adding serious unittesting for the new implementation.
>>
>
> Thx
>
>
> http://codereview.chromium.org/6862002/
>

[Sigurður Ásgeirsson, 2011-04-15 17:41:45]

Looks pretty good overall, I have a bunch of nits.

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

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:49: const char* GetPhaseStr(TraceEventPhase phase) {
nit: one blank line after opening namespace.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:50: if (phase== TRACE_EVENT_PHASE_BEGIN) {
nit: phase ==

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:50: if (phase== TRACE_EVENT_PHASE_BEGIN) {
perhaps this is better phrased as a switch?

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:61: }
nit: one blank line before closing namespace
}  // namespace

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:75: void TraceEvent::AppendAsJSON(std::string* out,
First input args, then output args:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:89: int nargs = 0;
some running commentary would make this function easier to read.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:96: const char* phaseStr = GetPhaseStr(phase);
phaseStr->phase_str

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:132: {
nit: bracket placement.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:141: // TODO(nduca): replace with a hash_map.
Yes, suggest adding an efficient name lookup before checkin.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:159:
base::subtle::NoBarrier_Store(&categories_[i]->enabled_,
better to encapsulate the flag in inline accessors to reduce clutter.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:164: for (size_t i = 0; i < categories_.size(); i++) {
copy paste loop, better to copy the test and do pre/post work conditionally.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:175: if (enabled_) {
nit: no brackets for an uncomplicated one-line compound statement.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:192: void TraceLog::FlushWithLockAlreadyHeld() {
lock_.AssertAcquired()?

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:198: output_callback_->Run(json_events);
I really don't like this - e.g. calling out of module with a lock held, on the
usual principles. I would suggest refactoring such that you only do acquire the
events and teh callback under the lock, with e.g. logged_events_.swap(), then
proceed with stringification and everything else outside the lock.
If you feel that ordering is important to the user, I'd suggest adding a
monotonically increasing flush count that you also acquire under the lock, and
pass on to the callback.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:208: const char* arg1name, const char* arg1val,
arg1_name, arg1_val, etc

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:216: //static_cast<unsigned
long>(base::GetCurrentProcId()),
?

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

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.h#new...
base/debug/trace_event.h:93: #define TRACE_EVENT2(category, name, arg1name,
arg1val, arg2name, arg2val) \
If I'm reading this correctly, these static pointers will dangle post-AtExit
processing, as the TraceLog singleton will deallocate all the categories AtExit?
This is worth a warning to the user at least, and is typical for the tradeoffs
between performance and robustness.
A unittest can be used to shed light on the post-AtExit behavior of this, and if
my reading is incorrect, then I'd like to see a test that makes sure this
doesn't regress.
Since the TRACE_EVENT macros are likely to get used by all and sundry, I find it
more likely than not that this will cause intolerable trouble :(.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.h#new...
base/debug/trace_event.h:100: base::debug::TRACE_EVENT_PHASE_BEGIN, \
a unittest to validate arguments are not evaluated unless the trace category is
enabled would be an excellent idea for maintainability.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.h#new...
base/debug/trace_event.h:194: volatile base::subtle::Atomic32 enabled_;
maybe wrap this in an inline accessor used by the macros?

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.h#new...
base/debug/trace_event.h:208: // OutputCallback whenever the logging system
decides to flush. This
logging system -> tracing system?

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.h#new...
base/debug/trace_event.h:212: static void AppendAsJSON(std::string* out,
one line comment for method purpose and/or usage?

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.h#new...
base/debug/trace_event.h:221: unsigned long processId;
nit: process_id, thread_id, arg_names, arg_values

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.h#new...
base/debug/trace_event.h:238: // TODO(nduca) Replaced with an
Enable/DisableCategory() that
missing colon after TODO, 
nduca->shceib?
Replaced -> Replace

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.h#new...
base/debug/trace_event.h:239: // implicitly controls the global logging state.
control->controls?

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.h#new...
base/debug/trace_event.h:244: // silently dropped.
Add a note that this takes ownership of the callback.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.h#new...
base/debug/trace_event.h:249: //  output callback.
nit: one space only after //

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.h#new...
base/debug/trace_event.h:287: // TODO(nduca): switch to per-thread tace buffers
to reduce thread
tace->trace?

[scheib, 2011-04-21 17:47:18]

Noting some test suggestions siggi brought up in a chat:
- Function even if destroyed
  - test via base::AtExitManager
- lazy argument evaluation (tracing off, no calls made)
- resurrection of static singleton

[scheib, 2011-04-27 21:20:10]

Tests implemented (less the lazy evaluation, issue filed).
nits addressed.
Flush holding a lock over the callback fixed.

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

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:49: const char* GetPhaseStr(TraceEventPhase phase) {
On 2011/04/15 17:41:45, Ruðrugis wrote:
> nit: one blank line after opening namespace.

Done.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:50: if (phase== TRACE_EVENT_PHASE_BEGIN) {
On 2011/04/15 17:41:45, Ruðrugis wrote:
> nit: phase == 

Done.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:50: if (phase== TRACE_EVENT_PHASE_BEGIN) {
On 2011/04/15 17:41:45, Ruðrugis wrote:
> perhaps this is better phrased as a switch?

Done.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:50: if (phase== TRACE_EVENT_PHASE_BEGIN) {
On 2011/04/15 17:41:45, Ruðrugis wrote:
> nit: phase == 

Done.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:61: }
On 2011/04/15 17:41:45, Ruðrugis wrote:
> nit: one blank line before closing namespace
> }  // namespace

Done.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:75: void TraceEvent::AppendAsJSON(std::string* out,
On 2011/04/15 17:41:45, Ruðrugis wrote:
> First input args, then output args:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Done.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:89: int nargs = 0;
On 2011/04/15 17:41:45, Ruðrugis wrote:
> some running commentary would make this function easier to read.

Done.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:96: const char* phaseStr = GetPhaseStr(phase);
On 2011/04/15 17:41:45, Ruðrugis wrote:
> phaseStr->phase_str

Done.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:132: {
On 2011/04/15 17:41:45, Ruðrugis wrote:
> nit: bracket placement.

Done.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:141: // TODO(nduca): replace with a hash_map.
On 2011/04/15 17:41:45, Ruðrugis wrote:
> Yes, suggest adding an efficient name lookup before checkin.

Categories moved to static array. Categories are cached at trace call sites, so
look up performance doesn't seem important to optimize here.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:159:
base::subtle::NoBarrier_Store(&categories_[i]->enabled_,
On 2011/04/15 17:41:45, Ruðrugis wrote:
> better to encapsulate the flag in inline accessors to reduce clutter.

Removed, changed to just a bool.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:164: for (size_t i = 0; i < categories_.size(); i++) {
On 2011/04/15 17:41:45, Ruðrugis wrote:
> copy paste loop, better to copy the test and do pre/post work conditionally.

Done.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:175: if (enabled_) {
On 2011/04/15 17:41:45, Ruðrugis wrote:
> nit: no brackets for an uncomplicated one-line compound statement.

Done.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:192: void TraceLog::FlushWithLockAlreadyHeld() {
On 2011/04/15 17:41:45, Ruðrugis wrote:
> lock_.AssertAcquired()?

Done.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:198: output_callback_->Run(json_events);
On 2011/04/15 17:41:45, Ruðrugis wrote:
> I really don't like this - e.g. calling out of module with a lock held, on the
> usual principles. I would suggest refactoring such that you only do acquire
the
> events and teh callback under the lock, with e.g. logged_events_.swap(), then
> proceed with stringification and everything else outside the lock.
> If you feel that ordering is important to the user, I'd suggest adding a
> monotonically increasing flush count that you also acquire under the lock, and
> pass on to the callback.

Done.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:208: const char* arg1name, const char* arg1val,
On 2011/04/15 17:41:45, Ruðrugis wrote:
> arg1_name, arg1_val, etc

Done.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:216: //static_cast<unsigned
long>(base::GetCurrentProcId()),
On 2011/04/15 17:41:45, Ruðrugis wrote:
> ?

Done.

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

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.h#new...
base/debug/trace_event.h:93: #define TRACE_EVENT2(category, name, arg1name,
arg1val, arg2name, arg2val) \
On 2011/04/15 17:41:45, Ruðrugis wrote:
> If I'm reading this correctly, these static pointers will dangle post-AtExit
> processing, as the TraceLog singleton will deallocate all the categories
AtExit?
> This is worth a warning to the user at least, and is typical for the tradeoffs
> between performance and robustness.
> A unittest can be used to shed light on the post-AtExit behavior of this, and
if
> my reading is incorrect, then I'd like to see a test that makes sure this
> doesn't regress.
> Since the TRACE_EVENT macros are likely to get used by all and sundry, I find
it
> more likely than not that this will cause intolerable trouble :(.

Done. Categories come from a static pool. A test for at exit behavior exists.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.h#new...
base/debug/trace_event.h:100: base::debug::TRACE_EVENT_PHASE_BEGIN, \
On 2011/04/15 17:41:45, Ruðrugis wrote:
> a unittest to validate arguments are not evaluated unless the trace category
is
> enabled would be an excellent idea for maintainability.

Good test, captured in http://crbug.com/80709 can we omit it for the first land?

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.h#new...
base/debug/trace_event.h:194: volatile base::subtle::Atomic32 enabled_;
On 2011/04/15 17:41:45, Ruðrugis wrote:
> maybe wrap this in an inline accessor used by the macros?

Removed, it was not needed. Just a volatile bool now.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.h#new...
base/debug/trace_event.h:208: // OutputCallback whenever the logging system
decides to flush. This
On 2011/04/15 17:41:45, Ruðrugis wrote:
> logging system -> tracing system?

Done.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.h#new...
base/debug/trace_event.h:212: static void AppendAsJSON(std::string* out,
On 2011/04/15 17:41:45, Ruðrugis wrote:
> one line comment for method purpose and/or usage?

Done.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.h#new...
base/debug/trace_event.h:221: unsigned long processId;
On 2011/04/15 17:41:45, Ruðrugis wrote:
> nit: process_id, thread_id, arg_names, arg_values

Done.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.h#new...
base/debug/trace_event.h:238: // TODO(nduca) Replaced with an
Enable/DisableCategory() that
On 2011/04/15 17:41:45, Ruðrugis wrote:
> missing colon after TODO, 
> nduca->shceib?
> Replaced -> Replace
>  

Done.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.h#new...
base/debug/trace_event.h:239: // implicitly controls the global logging state.
On 2011/04/15 17:41:45, Ruðrugis wrote:
> control->controls?

Done.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.h#new...
base/debug/trace_event.h:244: // silently dropped.
On 2011/04/15 17:41:45, Ruðrugis wrote:
> Add a note that this takes ownership of the callback.

Done.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.h#new...
base/debug/trace_event.h:249: //  output callback.
On 2011/04/15 17:41:45, Ruðrugis wrote:
> nit: one space only after //

Done.

http://codereview.chromium.org/6862002/diff/8002/base/debug/trace_event.h#new...
base/debug/trace_event.h:287: // TODO(nduca): switch to per-thread tace buffers
to reduce thread
On 2011/04/15 17:41:45, Ruðrugis wrote:
> tace->trace?

Done.

[scheib, 2011-04-27 23:01:22]

Some of the build bots flaked out on my new tests - which I'll dig into.

I'd still appreciate review, there have been a lot of changes to trace_event
from gpu_trace_event, and test structure.

[Sigurður Ásgeirsson, 2011-04-28 13:17:16]

Hi Vincent,

I have an office move today and some stuff I need to get through before the
end of the week, so I don't think I'll get to this 'till next week, alas.

Siggi

On Wed, Apr 27, 2011 at 7:01 PM, <scheib@chromium.org> wrote:

> Some of the build bots flaked out on my new tests - which I'll dig into.
>
> I'd still appreciate review, there have been a lot of changes to
> trace_event
> from gpu_trace_event, and test structure.
>
>
>
> http://codereview.chromium.org/6862002/
>

[Sigurður Ásgeirsson, 2011-05-02 19:28:41]

Hi Vincent,

I took another quick look, it looks much better, but there are still a couple of
issues. I don't understand why you invoke the callback notifications under the
lock, this creates deadlock potential.
Also, this version won't leak, but as is it's likely to explode hard in the
field, plus you need some more work on how you deal with category storage.

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

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:28: static TraceCategory
g_categories[TRACE_EVENT_MAX_CATEGORIES];
This will unfortunately not work, and will not pass review, see
[http://www.chromium.org/developers/coding-style#Static_Variables]. Using static
storage for the class instances is the right thing to do, but as-is you have an
initialization race between the C++ runtime and the user of this array - which
could legitimately be called from a static initializer or during destruction.
I'm afraid you'll have to manage your storage manually, and/or make sure the
TraceCategories are PODs.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:52: free(value_.as_string);
Per
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/base/string_u...:

// Chromium code style is to not use malloc'd strings; this is only for use
// for interaction with APIs that require it.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:60: DCHECK(sizeof(value_) == sizeof(uint64));
COMPILE_ASSERT?

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:66: // Copy all 64 bits for all other types.
Move the assert from above here, by way of documentation as to why this works?

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:80: return (value_.as_uint == rhs.value_.as_uint);
mmmm, are you sure that doubles have a canonical representation for all possible
values?
Do you need equality comparison to begin with?

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:92: base::snprintf(temp_string, sizeof(temp_string),
"%llu",
nit: sizeof->arraysize

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:137: case TRACE_EVENT_PHASE_BEGIN:
nit: see style guide for indentation:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Loops_...

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:144: DCHECK(false);
NOTREACHED() << "Some comment";?

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:162: 
nit: spurious line.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:165: size_t start,
nit: align to bracket.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:181:
"{\"cat\":\"%s\",\"pid\":%i,\"tid\":%i,\"ts\":%lld,"
nit: the JSON won't be well-formed if any of the strings to %s contain quotes,
is there an assert or a check that prevents this someplace?

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:236: CHECK(g_category_index <
TRACE_EVENT_MAX_CATEGORIES) <<
This seems quite severe for release builds.
Assuming that there's a few little used nookies and crannies that refer to
distinct trace categories, you might expect them only to get exercised under
long and varied use, which is only likely to happen in release builds in the
wild... 
(nobody said it would be easy :).

I'd suggest a DCHECK, but logging error and returning a dummy category on
spillover in release builds. What do you think?

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:295: output_callback_->Run(json_events);
You're still calling out of module here with the lock held, was that the intent?
This is generally frowned upon with event notification machinery, as it
typically creates deadlock potential.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:332: buffer_full_callback_->Run();
you're calling out of module here under a lock also?

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:352: "id", StringPrintf("%p", id).c_str(),
nit: wonky indent.

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

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.h#ne...
base/debug/trace_event.h:40: // On Windows, the TRACE_EVENT output to ETW.
nit: abortive comment?

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.h#ne...
base/debug/trace_event.h:96: #define TRACE_EVENT2(category, name, arg1name,
arg1val, arg2name, arg2val) \
the name arguments here are expected to have infinite lifetime? This warrants a
comment?

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.h#ne...
base/debug/trace_event.h:189: void set(const char* name, bool enabled) {
more descriptive name here, probably needs to start with a capital letter, as
this is not a simple accessor:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.h#ne...
base/debug/trace_event.h:196: volatile bool enabled_;
still ought to wrap this in an accessor,
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Access...

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.h#ne...
base/debug/trace_event.h:228: value_.as_uint = 0ull; // zero all bits
why single out a few cases for this nulling?

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.h#ne...
base/debug/trace_event.h:264: value_.as_string = base::strdup(rhs);
Sorry, but no.
See comment at
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/base/string_u...

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

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:19: namespace {
is there commonly an anonymous namespace nested inside other namespaces? I
haven't seen this before, but if it's common practice in other tests, it's fine.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:21: class TraceEventTestFixture: public
testing::Test {
nit: spaces both side of : 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_...

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:23: TraceEventTestFixture() {};
nit: no semicolon after the body.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:42: NewCallback(this,
&TraceEventTestFixture::OnTraceDataCollected));
nit: 4 space indent on continuation.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:46: if (0 && "Enable for visualizing what
data was collected and parsed") {
either remove, or e.g. control through the command line (CommandLine::HasSwitch
is your friend).

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:55: EXPECT_TRUE(root.get());
ASSERT_TRUE will return from a void function on failure.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:107: DictionaryValue* FindTraceEntry(const
ListValue& trace_parsed,
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:130: void
DataCaptured_CallTraces(WaitableEvent* task_complete_event) {
why the _ in the function name?

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:348: { // scope to destroy singleton
nice test!. It could do with a little bit of running commentary explaining what
you're testing and what you expect to happen at major transition points, since
the stuff you're testing here is pretty arcane - to put it mildly:).

[jbates, 2011-05-03 23:49:09]

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

http://codereview.chromium.org/6862002/diff/25001/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:46: value_.as_string_refptr = NULL;
it's harmless, but setting to NULL is redundant with line 49.

http://codereview.chromium.org/6862002/diff/25001/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:245: g_category_categories_exhasted->name =
g_category_categories_exhasted_str;
Could just put the literal strings here instead of defining globals.

http://codereview.chromium.org/6862002/diff/25001/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:264: return g_category_categories_exhasted;
This will silently fail on Release builds, which is I guess is usually what
people will be building when doing trace/perf tests.

That's why I used a CHECK instead of DCHECK.

http://codereview.chromium.org/6862002/diff/25001/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:314: output_callback_->Run(json_events);
There was a reason for the lock being held during the callback - it's possible
for the callback to be reset on another thread, so this can call a dangling
pointer.

The typical solution for this is recursive mutex, but we don't allow those, so
it seems that the only solution is to document that the callback must not
reenter. Any other ideas?

http://codereview.chromium.org/6862002/diff/25001/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:354: buffer_full_callback_->Run();
(same problem here with the dangling pointer)

[scheib, 2011-05-04 02:14:14]

siggi's and jbates' comments addressed. Please review modifications.

A test is still flaky and I'm chasing that down.

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

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:28: static TraceCategory
g_categories[TRACE_EVENT_MAX_CATEGORIES];
On 2011/05/02 19:28:41, Ruðrugis wrote:
> This will unfortunately not work, and will not pass review, see
> [http://www.chromium.org/developers/coding-style#Static_Variables]. Using
static
> storage for the class instances is the right thing to do, but as-is you have
an
> initialization race between the C++ runtime and the user of this array - which
> could legitimately be called from a static initializer or during destruction.
> I'm afraid you'll have to manage your storage manually, and/or make sure the
> TraceCategories are PODs.

Converted to POD.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:52: free(value_.as_string);
On 2011/05/02 19:28:41, Ruðrugis wrote:
> Per
>
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/base/string_u...:
> 
> // Chromium code style is to not use malloc'd strings; this is only for use
> // for interaction with APIs that require it.

Done.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:60: DCHECK(sizeof(value_) == sizeof(uint64));
On 2011/05/02 19:28:41, Ruðrugis wrote:
> COMPILE_ASSERT?

Refactored, assert not needed.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:66: // Copy all 64 bits for all other types.
On 2011/05/02 19:28:41, Ruðrugis wrote:
> Move the assert from above here, by way of documentation as to why this works?

Refactored, assert not needed.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:80: return (value_.as_uint == rhs.value_.as_uint);
On 2011/05/02 19:28:41, Ruðrugis wrote:
> mmmm, are you sure that doubles have a canonical representation for all
possible
> values?
> Do you need equality comparison to begin with?

Refactored, assert not needed.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:92: base::snprintf(temp_string, sizeof(temp_string),
"%llu",
On 2011/05/02 19:28:41, Ruðrugis wrote:
> nit: sizeof->arraysize

Done.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:137: case TRACE_EVENT_PHASE_BEGIN:
On 2011/05/02 19:28:41, Ruðrugis wrote:
> nit: see style guide for indentation:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Loops_...

Done.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:144: DCHECK(false);
On 2011/05/02 19:28:41, Ruðrugis wrote:
> NOTREACHED() << "Some comment";?

Done.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:162: 
On 2011/05/02 19:28:41, Ruðrugis wrote:
> nit: spurious line.

Done.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:165: size_t start,
On 2011/05/02 19:28:41, Ruðrugis wrote:
> nit: align to bracket.

Done.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:181:
"{\"cat\":\"%s\",\"pid\":%i,\"tid\":%i,\"ts\":%lld,"
On 2011/05/02 19:28:41, Ruðrugis wrote:
> nit: the JSON won't be well-formed if any of the strings to %s contain quotes,
> is there an assert or a check that prevents this someplace?

Done.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:236: CHECK(g_category_index <
TRACE_EVENT_MAX_CATEGORIES) <<
On 2011/05/02 19:28:41, Ruðrugis wrote:
> This seems quite severe for release builds.
> Assuming that there's a few little used nookies and crannies that refer to
> distinct trace categories, you might expect them only to get exercised under
> long and varied use, which is only likely to happen in release builds in the
> wild... 
> (nobody said it would be easy :).
> 
> I'd suggest a DCHECK, but logging error and returning a dummy category on
> spillover in release builds. What do you think?

Done.

Two special case categories added for post shutdown and category overflow.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:295: output_callback_->Run(json_events);
On 2011/05/02 19:28:41, Ruðrugis wrote:
> You're still calling out of module here with the lock held, was that the
intent?
> This is generally frowned upon with event notification machinery, as it
> typically creates deadlock potential.

This was an error and I've removed the lock. The callbacks must be (and are, see
trace_controller.cc) thread-safe.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:332: buffer_full_callback_->Run();
On 2011/05/02 19:28:41, Ruðrugis wrote:
> you're calling out of module here under a lock also?

Fixed.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:352: "id", StringPrintf("%p", id).c_str(),
On 2011/05/02 19:28:41, Ruðrugis wrote:
> nit: wonky indent.

Done.

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

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.h#ne...
base/debug/trace_event.h:40: // On Windows, the TRACE_EVENT output to ETW.
On 2011/05/02 19:28:41, Ruðrugis wrote:
> nit: abortive comment?

Done.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.h#ne...
base/debug/trace_event.h:96: #define TRACE_EVENT2(category, name, arg1name,
arg1val, arg2name, arg2val) \
On 2011/05/02 19:28:41, Ruðrugis wrote:
> the name arguments here are expected to have infinite lifetime? This warrants
a
> comment? 

Done.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.h#ne...
base/debug/trace_event.h:189: void set(const char* name, bool enabled) {
On 2011/05/02 19:28:41, Ruðrugis wrote:
> more descriptive name here, probably needs to start with a capital letter, as
> this is not a simple accessor:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Changed to POD

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.h#ne...
base/debug/trace_event.h:196: volatile bool enabled_;
On 2011/05/02 19:28:41, Ruðrugis wrote:
> still ought to wrap this in an accessor,
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Access...

Changed to POD

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.h#ne...
base/debug/trace_event.h:228: value_.as_uint = 0ull; // zero all bits
On 2011/05/02 19:28:41, Ruðrugis wrote:
> why single out a few cases for this nulling?

This code simplified / cut, since it is not needed currently. The reason was
that comparison could be done via largest integer type in the union if even
smaller types cleared the storage.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event.h#ne...
base/debug/trace_event.h:264: value_.as_string = base::strdup(rhs);
On 2011/05/02 19:28:41, Ruðrugis wrote:
> Sorry, but no.
> See comment at
>
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/base/string_u...

Done.

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

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:19: namespace {
On 2011/05/02 19:28:41, Ruðrugis wrote:
> is there commonly an anonymous namespace nested inside other namespaces? I
> haven't seen this before, but if it's common practice in other tests, it's
fine.

I think the same reasoning applies for anonymous NS even if it's nested?

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:21: class TraceEventTestFixture: public
testing::Test {
On 2011/05/02 19:28:41, Ruðrugis wrote:
> nit: spaces both side of : 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_...

Done.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:23: TraceEventTestFixture() {};
On 2011/05/02 19:28:41, Ruðrugis wrote:
> nit: no semicolon after the body.

Done.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:42: NewCallback(this,
&TraceEventTestFixture::OnTraceDataCollected));
On 2011/05/02 19:28:41, Ruðrugis wrote:
> nit: 4 space indent on continuation.

Done.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:46: if (0 && "Enable for visualizing what
data was collected and parsed") {
On 2011/05/02 19:28:41, Ruðrugis wrote:
> either remove, or e.g. control through the command line
(CommandLine::HasSwitch
> is your friend).

Done.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:55: EXPECT_TRUE(root.get());
On 2011/05/02 19:28:41, Ruðrugis wrote:
> ASSERT_TRUE will return from a void function on failure.

Done.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:107: DictionaryValue* FindTraceEntry(const
ListValue& trace_parsed,
On 2011/05/02 19:28:41, Ruðrugis wrote:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Done.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:130: void
DataCaptured_CallTraces(WaitableEvent* task_complete_event) {
On 2011/05/02 19:28:41, Ruðrugis wrote:
> why the _ in the function name?

Done.

http://codereview.chromium.org/6862002/diff/18073/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:348: { // scope to destroy singleton
On 2011/05/02 19:28:41, Ruðrugis wrote:
> nice test!. It could do with a little bit of running commentary explaining
what
> you're testing and what you expect to happen at major transition points, since
> the stuff you're testing here is pretty arcane - to put it mildly:).

Done.

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

http://codereview.chromium.org/6862002/diff/25001/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:46: value_.as_string_refptr = NULL;
On 2011/05/03 23:49:10, jbates wrote:
> it's harmless, but setting to NULL is redundant with line 49.

readability is easier with it.. ;)

http://codereview.chromium.org/6862002/diff/25001/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:245: g_category_categories_exhasted->name =
g_category_categories_exhasted_str;
On 2011/05/03 23:49:10, jbates wrote:
> Could just put the literal strings here instead of defining globals.

Would prefer them next to definition of the pointers, to explain what they are
there

http://codereview.chromium.org/6862002/diff/25001/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:264: return g_category_categories_exhasted;
On 2011/05/03 23:49:10, jbates wrote:
> This will silently fail on Release builds, which is I guess is usually what
> people will be building when doing trace/perf tests.
> 
> That's why I used a CHECK instead of DCHECK.


Yes, but as siggi pointed out, we don't want to crash users who are running
chrome for a long time and happen to hit a combination of code paths that
exhaust categories. In development those are less likely to be hit.

Instead, we should fix the viewer of trace data to look for the existence of
this category and warn that categories were maxed out.

http://codereview.chromium.org/6862002/diff/25001/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:314: output_callback_->Run(json_events);
On 2011/05/03 23:49:10, jbates wrote:
> There was a reason for the lock being held during the callback - it's possible
> for the callback to be reset on another thread, so this can call a dangling
> pointer.
> 
> The typical solution for this is recursive mutex, but we don't allow those, so
> it seems that the only solution is to document that the callback must not
> reenter. Any other ideas?


Fixed by moving to new callback form. Thx for help. ;)

http://codereview.chromium.org/6862002/diff/25001/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:354: buffer_full_callback_->Run();
On 2011/05/03 23:49:10, jbates wrote:
> (same problem here with the dangling pointer)

Done.

[Sigurður Ásgeirsson, 2011-05-04 18:58:08]

sweet, one more round of nits. Last one I hope...

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

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:23: #define TRACE_EVENT_BUFFER_SIZE 500000
const size_t, and ditto for below.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:28: static TraceCategory
g_categories[TRACE_EVENT_MAX_CATEGORIES];
nit: you can compile-initialize this to the strings with 
x[] = {
  { "name", false },
  { "name", false },
};

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:29: static TraceCategory* g_category_already_shutdown
= NULL;
you can compile-time initialize this to 
&g_categories[0] 
and
&g_categories[1]

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:30: static TraceCategory*
g_category_categories_exhasted = NULL;
exhasted->exhausted

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:32: "tracing already shutdown";
nit: indent 4 on continuation.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:35: static int g_category_index = 0;
and this to 2.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:125: default:
NOTREACHED()?

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:160: memset(&arg_names, 0, sizeof(arg_names));
because arrays degenerate to pointers, this caused me momentary pause. While
it's the same thing, I'd suggest:

memset(arg_names, 0, sizeof(arg_names)

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:182: DCHECK(!strchr(category->name, '"'));
nice, but this only fires (and pretty late) if tracing is enabled

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:235: AutoLock lock(lock_);
this looks like the perfect bottleneck to assert that name does not contain a
quote?

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:238: DCHECK(TRACE_EVENT_MAX_CATEGORIES >= 2);
COMPILE_ASSERT

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:242: DCHECK(!g_category_already_shutdown->enabled);
you can also COMPILE_ASSERT this

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:314: scoped_refptr<RefCountedString>
json_events_str_ptr =
why does the string need to be refcounted?

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:325: const char* file, int line,
nit: indent.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:367: const char* file, int line,
nit: indent.

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

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.h#ne...
base/debug/trace_event.h:49: #include "base/atomicops.h"
do you need this include still?

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.h#ne...
base/debug/trace_event.h:56: #include "base/callback.h"
duplicate include.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.h#ne...
base/debug/trace_event.h:58: #include <vector>
includes out of order.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.h#ne...
base/debug/trace_event.h:195: #define TRACE_MAX_NUM_ARGS 2
This should be a const int or an enum, DEFINEs don't scope well.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.h#ne...
base/debug/trace_event.h:326: size_t start,
nit: wonky indent.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.h#ne...
base/debug/trace_event.h:360: void SetOutputCallback(const OutputCallback& cb);
// takes ownership of cb
takes ownership is no longer true.

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

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:345: // Scope to contain the then destroy the
TraceLog singleton
supernit: comments start with a capital letter and end with a full stop.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:360: "is recorded 2; system has been
enabled");
nit: indent

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:391: "not recorded; system shutdown");
nit: indent.

http://codereview.chromium.org/6862002/diff/32003/content/browser/trace_contr...
File content/browser/trace_controller.cc (right):

http://codereview.chromium.org/6862002/diff/32003/content/browser/trace_contr...
content/browser/trace_controller.cc:28:
base::debug::TraceLog::OutputCallback());
nit: 4 spaces for continuation.

http://codereview.chromium.org/6862002/diff/32003/content/browser/trace_contr...
content/browser/trace_controller.cc:186: const
scoped_refptr<base::debug::TraceLog::RefCountedString>&
nit: is there a typedef you can borrow to make this a little less verbose?

http://codereview.chromium.org/6862002/diff/32003/content/browser/trace_messa...
File content/browser/trace_message_filter.cc (right):

http://codereview.chromium.org/6862002/diff/32003/content/browser/trace_messa...
content/browser/trace_message_filter.cc:86: make_scoped_refptr(new
base::debug::TraceLog::RefCountedString(data)));
nit: indent

http://codereview.chromium.org/6862002/diff/32003/content/common/child_trace_...
File content/common/child_trace_message_filter.cc (right):

http://codereview.chromium.org/6862002/diff/32003/content/common/child_trace_...
content/common/child_trace_message_filter.cc:19:
base::debug::TraceLog::OutputCallback());
indent here and below.

[scheib, 2011-05-05 00:06:36]

brettw: added as a base/ owner.
siggi: nits fixed.

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

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:23: #define TRACE_EVENT_BUFFER_SIZE 500000
On 2011/05/04 18:58:08, Ruðrugis wrote:
> const size_t, and ditto for below.

Done.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:28: static TraceCategory
g_categories[TRACE_EVENT_MAX_CATEGORIES];
On 2011/05/04 18:58:08, Ruðrugis wrote:
> nit: you can compile-initialize this to the strings with 
> x[] = {
>   { "name", false },
>   { "name", false },
> };

Done.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:29: static TraceCategory* g_category_already_shutdown
= NULL;
On 2011/05/04 18:58:08, Ruðrugis wrote:
> you can compile-time initialize this to 
> &g_categories[0] 
> and
> &g_categories[1]

Done.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:30: static TraceCategory*
g_category_categories_exhasted = NULL;
On 2011/05/04 18:58:08, Ruðrugis wrote:
> exhasted->exhausted

Done.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:32: "tracing already shutdown";
On 2011/05/04 18:58:08, Ruðrugis wrote:
> nit: indent 4 on continuation.

Done.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:35: static int g_category_index = 0;
On 2011/05/04 18:58:08, Ruðrugis wrote:
> and this to 2.

Done.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:125: default:
On 2011/05/04 18:58:08, Ruðrugis wrote:
> NOTREACHED()?

Done.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:160: memset(&arg_names, 0, sizeof(arg_names));
On 2011/05/04 18:58:08, Ruðrugis wrote:
> because arrays degenerate to pointers, this caused me momentary pause. While
> it's the same thing, I'd suggest:
> 
> memset(arg_names, 0, sizeof(arg_names)

Done.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:182: DCHECK(!strchr(category->name, '"'));
On 2011/05/04 18:58:08, Ruðrugis wrote:
> nice, but this only fires (and pretty late) if tracing is enabled

Done.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:235: AutoLock lock(lock_);
On 2011/05/04 18:58:08, Ruðrugis wrote:
> this looks like the perfect bottleneck to assert that name does not contain a
> quote?

Done.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:238: DCHECK(TRACE_EVENT_MAX_CATEGORIES >= 2);
On 2011/05/04 18:58:08, Ruðrugis wrote:
> COMPILE_ASSERT

Done.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:242: DCHECK(!g_category_already_shutdown->enabled);
On 2011/05/04 18:58:08, Ruðrugis wrote:
> you can also COMPILE_ASSERT this

Done.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:314: scoped_refptr<RefCountedString>
json_events_str_ptr =
On 2011/05/04 18:58:08, Ruðrugis wrote:
> why does the string need to be refcounted?

We do not want to copy the string through the additional callbacks downstream.
Strings will be passed to the IO thread for sending via IPC, so the lifetime
exceeds this callback.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:325: const char* file, int line,
On 2011/05/04 18:58:08, Ruðrugis wrote:
> nit: indent.

Done.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:367: const char* file, int line,
On 2011/05/04 18:58:08, Ruðrugis wrote:
> nit: indent.

Done.

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

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.h#ne...
base/debug/trace_event.h:49: #include "base/atomicops.h"
On 2011/05/04 18:58:08, Ruðrugis wrote:
> do you need this include still?

Done.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.h#ne...
base/debug/trace_event.h:56: #include "base/callback.h"
On 2011/05/04 18:58:08, Ruðrugis wrote:
> duplicate include.

Done.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.h#ne...
base/debug/trace_event.h:58: #include <vector>
On 2011/05/04 18:58:08, Ruðrugis wrote:
> includes out of order.

Done.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.h#ne...
base/debug/trace_event.h:195: #define TRACE_MAX_NUM_ARGS 2
On 2011/05/04 18:58:08, Ruðrugis wrote:
> This should be a const int or an enum, DEFINEs don't scope well. 

Done.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.h#ne...
base/debug/trace_event.h:326: size_t start,
On 2011/05/04 18:58:08, Ruðrugis wrote:
> nit: wonky indent.

Done.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event.h#ne...
base/debug/trace_event.h:360: void SetOutputCallback(const OutputCallback& cb);
// takes ownership of cb
On 2011/05/04 18:58:08, Ruðrugis wrote:
> takes ownership is no longer true.

Done.

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

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:345: // Scope to contain the then destroy the
TraceLog singleton
On 2011/05/04 18:58:08, Ruðrugis wrote:
> supernit: comments start with a capital letter and end with a full stop.

Done.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:360: "is recorded 2; system has been
enabled");
On 2011/05/04 18:58:08, Ruðrugis wrote:
> nit: indent

Done.

http://codereview.chromium.org/6862002/diff/30048/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:391: "not recorded; system shutdown");
On 2011/05/04 18:58:08, Ruðrugis wrote:
> nit: indent.

Done.

http://codereview.chromium.org/6862002/diff/32003/content/browser/trace_contr...
File content/browser/trace_controller.cc (right):

http://codereview.chromium.org/6862002/diff/32003/content/browser/trace_contr...
content/browser/trace_controller.cc:28:
base::debug::TraceLog::OutputCallback());
On 2011/05/04 18:58:09, Ruðrugis wrote:
> nit: 4 spaces for continuation.

Done.

http://codereview.chromium.org/6862002/diff/32003/content/browser/trace_contr...
content/browser/trace_controller.cc:186: const
scoped_refptr<base::debug::TraceLog::RefCountedString>&
On 2011/05/04 18:58:09, Ruðrugis wrote:
> nit: is there a typedef you can borrow to make this a little less verbose?

Style guidance from jam is to not typedef scoped_refptr

http://codereview.chromium.org/6862002/diff/32003/content/browser/trace_messa...
File content/browser/trace_message_filter.cc (right):

http://codereview.chromium.org/6862002/diff/32003/content/browser/trace_messa...
content/browser/trace_message_filter.cc:86: make_scoped_refptr(new
base::debug::TraceLog::RefCountedString(data)));
On 2011/05/04 18:58:09, Ruðrugis wrote:
> nit: indent

Done.

http://codereview.chromium.org/6862002/diff/32003/content/common/child_trace_...
File content/common/child_trace_message_filter.cc (right):

http://codereview.chromium.org/6862002/diff/32003/content/common/child_trace_...
content/common/child_trace_message_filter.cc:19:
base::debug::TraceLog::OutputCallback());
On 2011/05/04 18:58:09, Ruðrugis wrote:
> indent here and below.

Done.

[Sigurður Ásgeirsson, 2011-05-05 14:58:55]

LGTM as far as the base/trace_event changes are concerned.

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

http://codereview.chromium.org/6862002/diff/30053/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:239: CHECK(!strchr(name, '"')) << "Names may not
contain double quote char";
I'd make this a DCHECK, as we want to catch developers, but not inconvenience
users?

http://codereview.chromium.org/6862002/diff/30053/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:360: const char* file, int line,
re-nit:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

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

http://codereview.chromium.org/6862002/diff/30053/base/debug/trace_event.h#ne...
base/debug/trace_event.h:52: #include <vector>
nit: not your doing, but std includes precede our own
[http://dev.chromium.org/developers/coding-style]

[jbates, 2011-05-05 16:40:31]

LGTM

[scheib, 2011-05-05 16:50:28]

Nits addressed.

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

http://codereview.chromium.org/6862002/diff/30053/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:239: CHECK(!strchr(name, '"')) << "Names may not
contain double quote char";
On 2011/05/05 14:58:56, Ruðrugis wrote:
> I'd make this a DCHECK, as we want to catch developers, but not inconvenience
> users?

Done.

http://codereview.chromium.org/6862002/diff/30053/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:360: const char* file, int line,
On 2011/05/05 14:58:56, Ruðrugis wrote:
> re-nit:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Done.

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

http://codereview.chromium.org/6862002/diff/30053/base/debug/trace_event.h#ne...
base/debug/trace_event.h:52: #include <vector>
On 2011/05/05 14:58:56, Ruðrugis wrote:
> nit: not your doing, but std includes precede our own
> [http://dev.chromium.org/developers/coding-style]

Done.

[brettw, 2011-05-05 17:38:03]

LGTM for base, I didn't really check the details.

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

http://codereview.chromium.org/6862002/diff/32059/base/debug/trace_event.h#ne...
base/debug/trace_event.h:191: enum TraceEventPhase {
Can you add a comment about what this is for?

[scheib, 2011-05-09 23:39:51]

Land attempt revealed some issues, which have been resolved with these changes.
See comments to bring you to relevant changes.

- Shared dll build required some dll import/export, unused method removal, and
dependency added to gpu.gyp

- Crash at shutdown required not calling callback on an object that was being
destroyed.

- Thread Sanitizer needed to be informed of our intentional data race.

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

http://codereview.chromium.org/6862002/diff/46001/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:284: logged_events_.clear();
Correcting a shutdown crash, there is no need to Flush() here, we really just
want to ensure that no lingering trace events are carried over, so it is
modified to a clear(); (Flush can cause events to be gathered and sent via
callbacks, but those might be on objects that are destructing and clearing their
callbacks from TraceLog).

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

http://codereview.chromium.org/6862002/diff/46001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:65: // ANNOTATE_BENIGN_RACE is used to suppress the
warning on the static category
ANNOTATE_BENIGN_RACE added to trace event calls, documentation for why is placed
here.

http://codereview.chromium.org/6862002/diff/46001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:124:
ANNOTATE_BENIGN_RACE(&TRACE_EVENT_UNIQUE_IDENTIFIER(catstatic), \
ANNOTATE_BENIGN_RACE added here.

http://codereview.chromium.org/6862002/diff/46001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:251: class BASE_API TraceValue {
Shared dll build fix.

http://codereview.chromium.org/6862002/diff/46001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:313: 
Removed unused operator!= operator==; Caused linker error on a win build.

http://codereview.chromium.org/6862002/diff/46001/content/common/child_trace_...
File content/common/child_trace_message_filter.cc (right):

http://codereview.chromium.org/6862002/diff/46001/content/common/child_trace_...
content/common/child_trace_message_filter.cc:17:
ChildTraceMessageFilter::~ChildTraceMessageFilter() {
Shutdown crash was caused by cycle of destructor -> set callback -> flush ->
callback to object being destroyed.

Moved clearing of callbacks to OnFilterRemoved

http://codereview.chromium.org/6862002/diff/46001/gpu/gpu.gyp
File gpu/gpu.gyp (right):

http://codereview.chromium.org/6862002/diff/46001/gpu/gpu.gyp#newcode83
gpu/gpu.gyp:83: '../base/base.gyp:base',
Shared DLL build fix

[jbates, 2011-05-10 00:04:11]

LGTM - pair programmed some of these changes.

On 2011/05/09 23:39:51, scheib wrote:
> Land attempt revealed some issues, which have been resolved with these
changes.
> See comments to bring you to relevant changes.
> 
> - Shared dll build required some dll import/export, unused method removal, and
> dependency added to gpu.gyp
> 
> - Crash at shutdown required not calling callback on an object that was being
> destroyed.
> 
> - Thread Sanitizer needed to be informed of our intentional data race.
> 
> http://codereview.chromium.org/6862002/diff/46001/base/debug/trace_event.cc
> File base/debug/trace_event.cc (right):
> 
>
http://codereview.chromium.org/6862002/diff/46001/base/debug/trace_event.cc#n...
> base/debug/trace_event.cc:284: logged_events_.clear();
> Correcting a shutdown crash, there is no need to Flush() here, we really just
> want to ensure that no lingering trace events are carried over, so it is
> modified to a clear(); (Flush can cause events to be gathered and sent via
> callbacks, but those might be on objects that are destructing and clearing
their
> callbacks from TraceLog).
> 
> http://codereview.chromium.org/6862002/diff/46001/base/debug/trace_event.h
> File base/debug/trace_event.h (right):
> 
>
http://codereview.chromium.org/6862002/diff/46001/base/debug/trace_event.h#ne...
> base/debug/trace_event.h:65: // ANNOTATE_BENIGN_RACE is used to suppress the
> warning on the static category
> ANNOTATE_BENIGN_RACE added to trace event calls, documentation for why is
placed
> here.
> 
>
http://codereview.chromium.org/6862002/diff/46001/base/debug/trace_event.h#ne...
> base/debug/trace_event.h:124:
> ANNOTATE_BENIGN_RACE(&TRACE_EVENT_UNIQUE_IDENTIFIER(catstatic), \
> ANNOTATE_BENIGN_RACE added here.
> 
>
http://codereview.chromium.org/6862002/diff/46001/base/debug/trace_event.h#ne...
> base/debug/trace_event.h:251: class BASE_API TraceValue {
> Shared dll build fix.
> 
>
http://codereview.chromium.org/6862002/diff/46001/base/debug/trace_event.h#ne...
> base/debug/trace_event.h:313: 
> Removed unused operator!= operator==; Caused linker error on a win build.
> 
>
http://codereview.chromium.org/6862002/diff/46001/content/common/child_trace_...
> File content/common/child_trace_message_filter.cc (right):
> 
>
http://codereview.chromium.org/6862002/diff/46001/content/common/child_trace_...
> content/common/child_trace_message_filter.cc:17:
> ChildTraceMessageFilter::~ChildTraceMessageFilter() {
> Shutdown crash was caused by cycle of destructor -> set callback -> flush ->
> callback to object being destroyed.
> 
> Moved clearing of callbacks to OnFilterRemoved
> 
> http://codereview.chromium.org/6862002/diff/46001/gpu/gpu.gyp
> File gpu/gpu.gyp (right):
> 
> http://codereview.chromium.org/6862002/diff/46001/gpu/gpu.gyp#newcode83
> gpu/gpu.gyp:83: '../base/base.gyp:base',
> Shared DLL build fix

[nduca, 2011-05-10 00:04:56]

LGTM

[Sigurður Ásgeirsson, 2011-05-10 13:44:52]

lgtm

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

http://codereview.chromium.org/6862002/diff/46001/base/debug/trace_event.h#ne...
base/debug/trace_event.h:57: // we tollerate some data loss while the system is
being enabled/disabled and
nit: tollerate -> tolerate

http://codereview.chromium.org/6862002/diff/46001/content/common/child_trace_...
File content/common/child_trace_message_filter.cc (right):

http://codereview.chromium.org/6862002/diff/46001/content/common/child_trace_...
content/common/child_trace_message_filter.cc:29: {
nit: move up to previous line