Added support to trace_event for passing static string arguments without copy

Added support to trace_event for passing static string arguments without copy

Occasionally you want to add some static string to a trace, such as the __FILE__ string, but don't want the overhead of a allocation/copy. This change let's you specify those as argument values via a new TraceValue type. You specify const char* types to get the no-copy behavior. std::string types are always copied. To force a const char*, specify TRACE_STR_COPY(str).

TEST=trace_event_unittest.cc

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

[nduca, 2011-08-29 21:09:36]

Cool!

[scheib, 2011-08-29 21:46:09]

LGTM

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

http://codereview.chromium.org/7767014/diff/2001/base/debug/trace_event.h#new...
base/debug/trace_event.h:366: return TraceValue(rhs, 0);
nit: Can you just call the string constructor and then set the type after? Would
elide the detail of the magic 2nd param to solve overloading, and the private
constructor?
E.g. this function calls
TraceValue t = TraceValue(rhs);
t.type_ = TRACE_TYPE_STATIC_STRING;
return t;

[jbates, 2011-08-29 22:22:20]

That's cleaner, thanks Vince.

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

http://codereview.chromium.org/7767014/diff/2001/base/debug/trace_event.h#new...
base/debug/trace_event.h:366: return TraceValue(rhs, 0);
On 2011/08/29 21:46:09, scheib wrote:
> nit: Can you just call the string constructor and then set the type after?
Would
> elide the detail of the magic 2nd param to solve overloading, and the private
> constructor?
> E.g. this function calls
> TraceValue t = TraceValue(rhs);
> t.type_ = TRACE_TYPE_STATIC_STRING;
> return t;

Done.

[jbates, 2011-08-29 22:49:34]

Owner's review, plz!

[Evan Martin, 2011-08-29 22:57:34]

Rather than the macro, why not make the char* ctor not copy and the string& ctor
copy?

Looking at code, there are countless examples of the "wrong" way to use this new
API:

http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/debug/trace_even...

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

http://codereview.chromium.org/7767014/diff/9001/base/debug/trace_event.cc#ne...
base/debug/trace_event.cc:184: alloc_size +=
GetAllocLength(arg2_val.as_string());
For others confused by this CL, this here is the important logic that
distinguishes TYPE_STRING from TYPE_STATIC_STRING.

[jbates, 2011-08-29 23:25:52]

On 2011/08/29 22:57:34, Evan Martin wrote:
> Rather than the macro, why not make the char* ctor not copy and the string&
ctor
> copy?

I think this is a good idea, but it could be dangerous to always assume const
char* is OK to not copy (string::c_str is one example). I would also be
concerned about string's implicit const char* constructor. With the current
approach, it defaults copy unless the user explicitly asks for a static string.

> Looking at code, there are countless examples of the "wrong" way to use this
new
> API:

Looks like it could use some examples of using the new API. Is that what you
mean?

> 
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/debug/trace_even...
> 
> http://codereview.chromium.org/7767014/diff/9001/base/debug/trace_event.cc
> File base/debug/trace_event.cc (right):
> 
>
http://codereview.chromium.org/7767014/diff/9001/base/debug/trace_event.cc#ne...
> base/debug/trace_event.cc:184: alloc_size +=
> GetAllocLength(arg2_val.as_string());
> For others confused by this CL, this here is the important logic that
> distinguishes TYPE_STRING from TYPE_STATIC_STRING.

[nduca, 2011-08-29 23:27:16]

Not to derail the review, but I kinda think Evan has a point on this one. I've
always found this part of trace event to be le-stink. I'd be really supportive
of anything that made this normative, even if it made the API have some
"gotchas" for newbies.

[Evan Martin, 2011-08-29 23:33:16]

On 2011/08/29 23:25:52, jbates wrote:
> I think this is a good idea, but it could be dangerous to always assume const
> char* is OK to not copy (string::c_str is one example).

This is true, but we always need to be vigilant about this.  Chrome style is
generally that we should be using std::strings in all APIs to make ownership
more clear, and when something takes a char* you need to be careful about it.

> I would also be
> concerned about string's implicit const char* constructor. 

I believe the rule is that the better match wins, and const char* will better
match a static string.


> > Looking at code, there are countless examples of the "wrong" way to use this
> new
> > API:
> 
> Looks like it could use some examples of using the new API. Is that what you
> mean?

Sorta.  Take a browse through the users of this API:

http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=TRAC...

Almost every caller is passing in static strings.  To use your the API
correctly, we'd need to wrap each of these with the macro indicating they don't
want a copy.

[jbates, 2011-08-29 23:44:05]

OK, happy to switch to const string& for copied argument values. It's not super
dangerous anyway since if someone screws up it will just output garbage into the
trace json data.

> Almost every caller is passing in static strings.  To use your the API
> correctly, we'd need to wrap each of these with the macro indicating they
don't
> want a copy.

Oh, I realize what you meant. Those examples are not actually the "wrong" way.
Those char* are only referenced, because they expected to be literal strings. We
only copy argument values (of the arg_name/arg_value pairs for TRACE_EVENT1 and
TRACE_EVENT2) that are char*. The exception is if the special _COPY macros are
used (explained in the Memory scoping section).

[jbates, 2011-08-30 01:47:21]

Latest CL uses std::string to indicate copy behavior and const char* indicates
reference behavior.

joth and jrg: please check your binding with trace_event to make sure that works
for you.

[scheib, 2011-08-30 16:16:02]

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

http://codereview.chromium.org/7767014/diff/9007/base/debug/trace_event.h#new...
base/debug/trace_event.h:114: } while(0)
Sorry, I'm confused why the changes were needed here? Also, it appears that the
legacy trace points are being discarded on non windows platforms? Aren't they
still handy to have? E.g. the events that come from V8 via webkit
(webkitclient_impl.cc)?

[jbates, 2011-08-30 17:01:51]

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

http://codereview.chromium.org/7767014/diff/9007/base/debug/trace_event.h#new...
base/debug/trace_event.h:114: } while(0)
On 2011/08/30 16:16:02, scheib wrote:
> Sorry, I'm confused why the changes were needed here? Also, it appears that
the
> legacy trace points are being discarded on non windows platforms? Aren't they
> still handy to have? E.g. the events that come from V8 via webkit
> (webkitclient_impl.cc)?

The behavior should be the same as before (unless I've made a mistake). This was
just changed to reuse our existing macros, since our macros now support
std::string arguments. TraceLog::AddTraceEventEtw converted std::string to const
char*. Instead, these macros just pass through whether it's std::string or const
char*, since we now support both.

[John Grabowski, 2011-08-30 18:13:38]

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

http://codereview.chromium.org/7767014/diff/9007/base/debug/trace_event.h#new...
base/debug/trace_event.h:55: //     TRACE_EVENT1("category", "event1",
Comment should have COPY in the name (2 spots)

http://codereview.chromium.org/7767014/diff/9007/base/debug/trace_event.h#new...
base/debug/trace_event.h:58: //     TRACE_EVENT1("category", "event2",
This actually does not work for us.  We convert some string types to const char*
but still need them copied.  Will send details in email.

[jbates, 2011-08-30 22:21:35]

jrg: I fixed the _COPY macros to always copy string arg values.

I also added a TRACE_STR_COPY macro for when we need to force a copy of const
char* argument values.

Evan: how's it look?

[joth, 2011-08-31 13:43:33]

LG - 2 minor suggestions...

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

http://codereview.chromium.org/7767014/diff/20001/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:89: *out += as_string()? as_string() : "NULL";
nit: space before ?

http://codereview.chromium.org/7767014/diff/20001/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:184: // We always take a copy of string arg_vals, even
if |copy| was not set.
this comment is a little misleading now.
// We only take a copy of arg_vals if they are of type string (not static
string), regardless of the |copy| flag.

[jbates, 2011-08-31 17:11:11]

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

http://codereview.chromium.org/7767014/diff/20001/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:89: *out += as_string()? as_string() : "NULL";
On 2011/08/31 13:43:33, joth wrote:
> nit: space before ?

Done.

http://codereview.chromium.org/7767014/diff/20001/base/debug/trace_event.cc#n...
base/debug/trace_event.cc:184: // We always take a copy of string arg_vals, even
if |copy| was not set.
On 2011/08/31 13:43:33, joth wrote:
> this comment is a little misleading now.
> // We only take a copy of arg_vals if they are of type string (not static
> string), regardless of the |copy| flag.

Done.

[jbates, 2011-09-02 00:47:08]

On 2011/08/31 17:11:11, jbates wrote:
> http://codereview.chromium.org/7767014/diff/20001/base/debug/trace_event.cc
> File base/debug/trace_event.cc (right):
> 
>
http://codereview.chromium.org/7767014/diff/20001/base/debug/trace_event.cc#n...
> base/debug/trace_event.cc:89: *out += as_string()? as_string() : "NULL";
> On 2011/08/31 13:43:33, joth wrote:
> > nit: space before ?
> 
> Done.
> 
>
http://codereview.chromium.org/7767014/diff/20001/base/debug/trace_event.cc#n...
> base/debug/trace_event.cc:184: // We always take a copy of string arg_vals,
even
> if |copy| was not set.
> On 2011/08/31 13:43:33, joth wrote:
> > this comment is a little misleading now.
> > // We only take a copy of arg_vals if they are of type string (not static
> > string), regardless of the |copy| flag.
> 
> Done.

Evan: LG?

[jbates, 2011-09-06 17:17:00]

owners ping...

[Evan Martin, 2011-09-06 17:45:29]

I hope you don't mind, I put the subject of the review int he review description
(the subject is lost when we commit)

LGTM