Add trace code to track all posted tasks in message_loop.

Add trace code to track all posted tasks in message_loop and WorkerThreads (non-official builds only).

It's very helpful to understand what chrome is doing at runtime. Sometimes a thread in chrome does something expensive that causes a frame hitch. With this change, any expensive task will show up clearly in traces, with the file/function of where the task was posted.

TEST=go to about:tracing, run a trace and notice that all tasks are traced.

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

[jbates, 2011-08-31 00:40:48]

http://codereview.chromium.org/7778033/diff/1/base/message_loop.h
File base/message_loop.h (right):

http://codereview.chromium.org/7778033/diff/1/base/message_loop.h#newcode434
base/message_loop.h:434: tracked_objects::Location birth_location;
Location is only three pointers, so if we store the whole thing here we know
what code posted tasks that show up in traces. (Also moved it up here to reduce
struct padding.)

http://codereview.chromium.org/7778033/diff/1/base/tracked.h
File base/tracked.h (right):

http://codereview.chromium.org/7778033/diff/1/base/tracked.h#newcode48
base/tracked.h:48: const char* function_name,
I really wanted this to be "FILE:LINE:FUNCTION" to remove another pointer.
Unfortunately, __FUNCTION__ is not expanded by the preprocessor, so it can not
be concatenated at preprocess time.

http://codereview.chromium.org/7778033/diff/1/base/tracked.h#newcode61
base/tracked.h:61: return file_line_ < other.file_line_;
If file_line is the same, function is the same, so no point in comparing
function.

[darin (slow to review), 2011-08-31 21:40:54]

http://codereview.chromium.org/7778033/diff/1/base/message_loop.cc
File base/message_loop.cc (right):

http://codereview.chromium.org/7778033/diff/1/base/message_loop.cc#newcode462
base/message_loop.cc:462: TRACE_EVENT2("base", "MessageLoop::RunTask",
does this cause the FROM_HERE strings to start getting linked into chrome.dll? 
do you know how that impacts the codesize of chrome.dll?

http://codereview.chromium.org/7778033/diff/1/base/message_loop.cc#newcode475
base/message_loop.cc:475: pending_task.birth_location.program_counter();
nit: 4 space indent for line continuation

http://codereview.chromium.org/7778033/diff/1/base/message_loop.h
File base/message_loop.h (right):

http://codereview.chromium.org/7778033/diff/1/base/message_loop.h#newcode434
base/message_loop.h:434: tracked_objects::Location birth_location;
So post_births has this same information:  post_births->location()

It seems like we should somehow rectify these two uses of location tracking. 
You should really discuss this with Jar since he created the tracked object
system.

[jar (doing other things), 2011-09-01 18:17:51]

http://codereview.chromium.org/7778033/diff/1/base/tracked.cc
File base/tracked.cc (right):

http://codereview.chromium.org/7778033/diff/1/base/tracked.cc#newcode46
base/tracked.cc:46: base::StringPiece str(file_line_);
nit: style guide suggests abbreviations be avoided.
Perhaps:
str --> file_line


Same thing on line 41.

http://codereview.chromium.org/7778033/diff/1/base/tracked.h
File base/tracked.h (right):

http://codereview.chromium.org/7778033/diff/1/base/tracked.h#newcode48
base/tracked.h:48: const char* function_name,
There is a memory cost of having FILE:LINE, as it causes unique static strings
to be allocated. The original form only caused one static string per file that
posted a task.  This code causes the compounded string to be allocated.

You say you wanted to reduce this to one pointer, and that is presumably to make
each element in your history queue smaller.  

Having the extra data in each pointed to object would mean more static memory
allocated.  You should look closely at the tradeoff, for the queue size you
select.

I'm not fully clear about a strong advantage one way or the other, and I'd like
to understand the justification for the choice you made.

The distinctive strings also provide for a faster matching system, but I don't
know if that was a bottleneck (perf issue).  It may be even less of an issue if
you don't have to do matching (which I don't think needs to be done if you just
queue the history, rather than tally stats in real time).

On 2011/08/31 00:40:48, jbates wrote:
> I really wanted this to be "FILE:LINE:FUNCTION" to remove another pointer.
> Unfortunately, __FUNCTION__ is not expanded by the preprocessor, so it can not
> be concatenated at preprocess time.

http://codereview.chromium.org/7778033/diff/1/base/tracked.h#newcode61
base/tracked.h:61: return file_line_ < other.file_line_;
I think this is not true when templates are involed.  The function name can
vary, even if the file and line number are the same.  I think it is a mistake to
change this code.  Function name comparison is needed.

On 2011/08/31 00:40:48, jbates wrote:
> If file_line is the same, function is the same, so no point in comparing
> function.

http://codereview.chromium.org/7778033/diff/1/base/tracked.h#newcode80
base/tracked.h:80: const void* program_counter_;
Why did you decide these didn't need to be constant pointers as well as pointers
to constant data?

http://codereview.chromium.org/7778033/diff/1/base/tracked.h#newcode89
base/tracked.h:89: __FILE__ ":" STRINGIZE(__LINE__), __FUNCTION__, \
As mentioned, this will notably increase memory utilization for the static
strings.  It is not clear this is a positive tradeoff.

http://codereview.chromium.org/7778033/diff/1/base/tracked_objects.cc
File base/tracked_objects.cc (right):

http://codereview.chromium.org/7778033/diff/1/base/tracked_objects.cc#newcode868
base/tracked_objects.cc:868: if
(sample.location().file_name().find(required_.c_str()) ==
Since you have transition to passing SringPieces objects, you no longer need the
.c_str() in the argument (to find).

http://codereview.chromium.org/7778033/diff/1/chrome/browser/sync/util/logging.h
File chrome/browser/sync/util/logging.h (right):

http://codereview.chromium.org/7778033/diff/1/chrome/browser/sync/util/loggin...
chrome/browser/sync/util/logging.h:25:
logging::LogMessage(from_here.file_name().as_string().c_str(),        \
I'm not clear on how often this logging is done... but this conversion to a
string and back may become a perf issue.

[jbates, 2011-09-01 23:01:52]

With this CL, size on linux increases in text by 80KB, but bss and data remain
about the same.

http://codereview.chromium.org/7778033/diff/1/base/message_loop.cc
File base/message_loop.cc (right):

http://codereview.chromium.org/7778033/diff/1/base/message_loop.cc#newcode475
base/message_loop.cc:475: pending_task.birth_location.program_counter();
On 2011/08/31 21:40:55, darin wrote:
> nit: 4 space indent for line continuation

Done.

http://codereview.chromium.org/7778033/diff/1/base/tracked.cc
File base/tracked.cc (right):

http://codereview.chromium.org/7778033/diff/1/base/tracked.cc#newcode46
base/tracked.cc:46: base::StringPiece str(file_line_);
On 2011/09/01 18:17:51, jar wrote:
> nit: style guide suggests abbreviations be avoided.
> Perhaps:
> str --> file_line
> 
> 
> Same thing on line 41.

Done.

http://codereview.chromium.org/7778033/diff/1/base/tracked.h
File base/tracked.h (right):

http://codereview.chromium.org/7778033/diff/1/base/tracked.h#newcode48
base/tracked.h:48: const char* function_name,
On 2011/09/01 18:17:51, jar wrote:
> There is a memory cost of having FILE:LINE, as it causes unique static strings
> to be allocated. The original form only caused one static string per file that
> posted a task.  This code causes the compounded string to be allocated.
> 
> You say you wanted to reduce this to one pointer, and that is presumably to
make
> each element in your history queue smaller.  
>
> Having the extra data in each pointed to object would mean more static memory
> allocated.  You should look closely at the tradeoff, for the queue size you
> select.
> 
> I'm not fully clear about a strong advantage one way or the other, and I'd
like
> to understand the justification for the choice you made.
> 
> The distinctive strings also provide for a faster matching system, but I don't
> know if that was a bottleneck (perf issue).  It may be even less of an issue
if
> you don't have to do matching (which I don't think needs to be done if you
just
> queue the history, rather than tally stats in real time).
> 
> On 2011/08/31 00:40:48, jbates wrote:
> > I really wanted this to be "FILE:LINE:FUNCTION" to remove another pointer.
> > Unfortunately, __FUNCTION__ is not expanded by the preprocessor, so it can
not
> > be concatenated at preprocess time.
> 


The only purpose I had in mind of merging these is to reduce the footprint of
Location. Part of this change adds Location to a frequently used object
PendingTask. We pass these objects around in a lot of places, so I thought it
would be a worthwhile tradeoff to slightly increase static memory. Originally I
thought I could remove the FUNCTION pointer as well (ie: it made more sense when
I thought I was cutting the size in half).

This change isn't required, but I think it is a slight improvement.

http://codereview.chromium.org/7778033/diff/1/base/tracked.h#newcode61
base/tracked.h:61: return file_line_ < other.file_line_;
On 2011/09/01 18:17:51, jar wrote:
> I think this is not true when templates are involed.  The function name can
> vary, even if the file and line number are the same.  I think it is a mistake
to
> change this code.  Function name comparison is needed.
> 
> On 2011/08/31 00:40:48, jbates wrote:
> > If file_line is the same, function is the same, so no point in comparing
> > function.
> 

OK, I'll add it back in. In linux/gcc, what I see is the undecorated method name
without template info, but it looks like some compilers may include template
info. Of course, __FUNCTION__ is non-standard, so if code depends on consistent
behavior here we should just remove __FUNCTION__ from Location.

http://codereview.chromium.org/7778033/diff/1/base/tracked.h#newcode80
base/tracked.h:80: const void* program_counter_;
On 2011/09/01 18:17:51, jar wrote:
> Why did you decide these didn't need to be constant pointers as well as
pointers
> to constant data?

operator= doesn't work if these pointers can't be set. Was there some reason to
prevent use of operator=?

http://codereview.chromium.org/7778033/diff/1/base/tracked.h#newcode89
base/tracked.h:89: __FILE__ ":" STRINGIZE(__LINE__), __FUNCTION__, \
On 2011/09/01 18:17:51, jar wrote:
> As mentioned, this will notably increase memory utilization for the static
> strings.  It is not clear this is a positive tradeoff.

I compared release builds with and without this CL, and found that the data size
is identical and bss size is up by 32 bytes.

http://codereview.chromium.org/7778033/diff/1/base/tracked_objects.cc
File base/tracked_objects.cc (right):

http://codereview.chromium.org/7778033/diff/1/base/tracked_objects.cc#newcode868
base/tracked_objects.cc:868: if
(sample.location().file_name().find(required_.c_str()) ==
On 2011/09/01 18:17:51, jar wrote:
> Since you have transition to passing SringPieces objects, you no longer need
the
> .c_str() in the argument (to find).

Done.

http://codereview.chromium.org/7778033/diff/1/chrome/browser/sync/util/logging.h
File chrome/browser/sync/util/logging.h (right):

http://codereview.chromium.org/7778033/diff/1/chrome/browser/sync/util/loggin...
chrome/browser/sync/util/logging.h:25:
logging::LogMessage(from_here.file_name().as_string().c_str(),        \
On 2011/09/01 18:17:51, jar wrote:
> I'm not clear on how often this logging is done... but this conversion to a
> string and back may become a perf issue.

I agree if this is common we should add StringPiece support to LogMessage. Those
arguments aren't evaluated unless logging is enabled though, and when logging is
on, there are lots of string and stream operations that should make this
negligible.

[jar (doing other things), 2011-09-03 00:26:33]

http://codereview.chromium.org/7778033/diff/1/base/tracked.h
File base/tracked.h (right):

http://codereview.chromium.org/7778033/diff/1/base/tracked.h#newcode80
base/tracked.h:80: const void* program_counter_;
I think it made it clear that the object was not going to change. I'm surprised
you were changing the internals.  Conceptually, I thought of a Location as a
constant, no different from "line 7."  


On 2011/09/01 23:01:52, jbates wrote:
> On 2011/09/01 18:17:51, jar wrote:
> > Why did you decide these didn't need to be constant pointers as well as
> pointers
> > to constant data?
> 
> operator= doesn't work if these pointers can't be set. Was there some reason
to
> prevent use of operator=?

http://codereview.chromium.org/7778033/diff/1/base/tracked.h#newcode88
base/tracked.h:88: #define FROM_HERE tracked_objects::Location( \
I'm not sure how this happened... but the goal is to have zero perf impact when
we're not tracking objects, such as in a release build.  Toward that end, I
historically defined stuff like FROM_HERE (when not tracking) to be something
with constant strings and values, so there would be no chance the compile would
waste time/space calculating anything.  In addition, I built the constructors to
ignore their arguments, further ensuring no possible impact.  

What happened to that plan/philosophy?  Do you know??

http://codereview.chromium.org/7778033/diff/1/base/tracked.h#newcode89
base/tracked.h:89: __FILE__ ":" STRINGIZE(__LINE__), __FUNCTION__, \
hmmm... the "cost" will appear in each file where there is more than one
PostTask. In that case, the file name (which seems to include the path relative
to some src root), will be replicated.  I guess the benefit will be that only a
pair of parameters (two pointers) can be used instead of a triplet (two pointers
and an int). I'm more than amazed that the difference is 32 bytes.

Was there less inlining?  Am I missing something??

On 2011/09/01 23:01:52, jbates wrote:
> On 2011/09/01 18:17:51, jar wrote:
> > As mentioned, this will notably increase memory utilization for the static
> > strings.  It is not clear this is a positive tradeoff.
> 
> I compared release builds with and without this CL, and found that the data
size
> is identical and bss size is up by 32 bytes.

http://codereview.chromium.org/7778033/diff/1/base/tracked.h#newcode117
base/tracked.h:117: base::TimeTicks tracked_birth_time() const { return
base::TimeTicks::Now(); }
You didn't just make this change... but....

My general goal was always that when TRACK_ALL_TASK_OBJECTS was not defined, we
would effectively compile away all our code, and have zero perf impact.  I'm not
sure if anyone is really calling this method, but a much more minimal
implementation would be:

return base::TimeTicks::TimeTicks();

Or even NOT_REACHED(), or CHECK(false);

[jbates, 2011-09-03 01:11:41]

http://codereview.chromium.org/7778033/diff/1/base/tracked.h
File base/tracked.h (right):

http://codereview.chromium.org/7778033/diff/1/base/tracked.h#newcode80
base/tracked.h:80: const void* program_counter_;
On 2011/09/03 00:26:33, jar wrote:
> I think it made it clear that the object was not going to change. I'm
surprised
> you were changing the internals.  Conceptually, I thought of a Location as a
> constant, no different from "line 7."  
> 

I see. I find it useful to be able to copy Location, for example if a class is
tracking the last Location it was called from, it would code:
obj.location_ = posted_from;

That only works with this change.

> 
> On 2011/09/01 23:01:52, jbates wrote:
> > On 2011/09/01 18:17:51, jar wrote:
> > > Why did you decide these didn't need to be constant pointers as well as
> > pointers
> > > to constant data?
> > 
> > operator= doesn't work if these pointers can't be set. Was there some reason
> to
> > prevent use of operator=?
>

http://codereview.chromium.org/7778033/diff/1/base/tracked.h#newcode88
base/tracked.h:88: #define FROM_HERE tracked_objects::Location( \
On 2011/09/03 00:26:33, jar wrote:
> I'm not sure how this happened... but the goal is to have zero perf impact
when
> we're not tracking objects, such as in a release build.  Toward that end, I
> historically defined stuff like FROM_HERE (when not tracking) to be something
> with constant strings and values, so there would be no chance the compile
would
> waste time/space calculating anything.  In addition, I built the constructors
to
> ignore their arguments, further ensuring no possible impact.  
> 
> What happened to that plan/philosophy?  Do you know??

I don't know the history of this code. The use cases I've seen are pretty heavy
weight - creating and enqueuing tasks mostly. FROM_HERE is passing and storing
three pointers (not sure the platform specifics of GetProgramCounter, but I'd
guess it's a simple register access). In comparison to the use cases, I don't
think that's too much overhead for enabling performance analysis in our
release/official builds. We can extract incredibly useful information from end
user traces.

http://codereview.chromium.org/7778033/diff/1/base/tracked.h#newcode89
base/tracked.h:89: __FILE__ ":" STRINGIZE(__LINE__), __FUNCTION__, \
On 2011/09/03 00:26:33, jar wrote:
> hmmm... the "cost" will appear in each file where there is more than one
> PostTask. In that case, the file name (which seems to include the path
relative
> to some src root), will be replicated.  I guess the benefit will be that only
a
> pair of parameters (two pointers) can be used instead of a triplet (two
pointers
> and an int). I'm more than amazed that the difference is 32 bytes.
> 
> Was there less inlining?  Am I missing something??

code/text size is separate, and it does go up by 80KB with this change as I
mentioned, but these strings aren't stored in the text section. The BSS size
went up 32 bytes, but that's irrelevant because that also wouldn't include these
strings (BSS is alloc'd and zero'd at runtime). I expected this change to
increase the size of the data section, but maybe the linker was being dumb
before and not merging equal strings. In any case, there aren't many files with
lots of FROM_HEREs so I think we're OK.

> 
> On 2011/09/01 23:01:52, jbates wrote:
> > On 2011/09/01 18:17:51, jar wrote:
> > > As mentioned, this will notably increase memory utilization for the static
> > > strings.  It is not clear this is a positive tradeoff.
> > 
> > I compared release builds with and without this CL, and found that the data
> size
> > is identical and bss size is up by 32 bytes.
>

http://codereview.chromium.org/7778033/diff/1/base/tracked.h#newcode117
base/tracked.h:117: base::TimeTicks tracked_birth_time() const { return
base::TimeTicks::Now(); }
On 2011/09/03 00:26:33, jar wrote:
> You didn't just make this change... but....
> 
> My general goal was always that when TRACK_ALL_TASK_OBJECTS was not defined,
we
> would effectively compile away all our code, and have zero perf impact.  I'm
not
> sure if anyone is really calling this method, but a much more minimal
> implementation would be:
> 
> return base::TimeTicks::TimeTicks();
> 
> Or even NOT_REACHED(), or CHECK(false);

I believe ajwong is considering removing this class altogether.

[jbates, 2011-09-07 00:05:18]

Finally found the source of windows test errors. Fixed in another CL:
http://codereview.chromium.org/7840017/

LG?

[jbates, 2011-09-07 17:24:18]

jar: LG?

[joth, 2011-09-15 10:38:09]

Thanks for adding this, I've already found these traces really useful in my own
work.

http://codereview.chromium.org/7778033/diff/15001/base/message_loop.h
File base/message_loop.h (right):

http://codereview.chromium.org/7778033/diff/15001/base/message_loop.h#newcode434
base/message_loop.h:434: tracked_objects::Location posted_from;
I managed to avoid this one by using post_births->location() (but, bearing in
mind post_births maybe NULL esp early in startup)

This does make it limited to TRACK_ALL_TASK_OBJECTS builds though, but that
seemed a reasonable trade off?

http://codereview.chromium.org/7778033/diff/15001/base/tracked.cc
File base/tracked.cc (right):

http://codereview.chromium.org/7778033/diff/15001/base/tracked.cc#newcode54
base/tracked.cc:54: return base::StringPiece(function_name_);
nit: is there any point returning this as a string piece when the caller just
wants to call .data() on it? The implicit strlen() call here is then wasted
effort, and the caller has to make an assumption that data() will be null
terminated. (c.f. it's easy for the caller to promote char* to StringPiece if
they need to).

http://codereview.chromium.org/7778033/diff/15001/base/tracked.h
File base/tracked.h (right):

http://codereview.chromium.org/7778033/diff/15001/base/tracked.h#newcode91
base/tracked.h:91: __FILE__ ":" STRINGIZE(__LINE__), __FUNCTION__, \
random musing:
if we passed in (sizeof(__FILE__) + sizeof( STRINGIZE(__LINE__) - 1) as an
additional param here, we'll get it as a compile time constant, and then we
could store file_line_ as a string piece with a compile-time generated length,
meaning we never need to run it searching for the null-term in the file_name()
and line_number() methods. 
which in turn brings them nearer to the hacker_style() requirement of being mere
getters; at the moment they feel rather like they should be FileName() style
methods.

[jar (doing other things), 2011-09-15 16:26:39]

http://codereview.chromium.org/7778033/diff/1/base/tracked.h
File base/tracked.h (right):

http://codereview.chromium.org/7778033/diff/1/base/tracked.h#newcode88
base/tracked.h:88: #define FROM_HERE tracked_objects::Location( \
Can you clarify how this is used? Is it sent up as part of crash dumps?  If not,
then I don't see how we'd justify much of any cost in an official build.

On 2011/09/03 01:11:41, jbates wrote:
> On 2011/09/03 00:26:33, jar wrote:
> > I'm not sure how this happened... but the goal is to have zero perf impact
> when
> > we're not tracking objects, such as in a release build.  Toward that end, I
> > historically defined stuff like FROM_HERE (when not tracking) to be
something
> > with constant strings and values, so there would be no chance the compile
> would
> > waste time/space calculating anything.  In addition, I built the
constructors
> to
> > ignore their arguments, further ensuring no possible impact.  
> > 
> > What happened to that plan/philosophy?  Do you know??
> 
> I don't know the history of this code. The use cases I've seen are pretty
heavy
> weight - creating and enqueuing tasks mostly. FROM_HERE is passing and storing
> three pointers (not sure the platform specifics of GetProgramCounter, but I'd
> guess it's a simple register access). In comparison to the use cases, I don't
> think that's too much overhead for enabling performance analysis in our
> release/official builds. We can extract incredibly useful information from end
> user traces.

http://codereview.chromium.org/7778033/diff/15001/base/tracked.h
File base/tracked.h (right):

http://codereview.chromium.org/7778033/diff/15001/base/tracked.h#newcode91
base/tracked.h:91: __FILE__ ":" STRINGIZE(__LINE__), __FUNCTION__, \
I think that sizeof(__FILE__) == sizeof(char*)

I don't think you get what you're hoping for.  You may perchance be thinking
about:

static array[] = __FILE__;
then sizeof(array) == strlen(array) I think.

On 2011/09/15 10:38:09, joth wrote:
> random musing:
> if we passed in (sizeof(__FILE__) + sizeof( STRINGIZE(__LINE__) - 1) as an
> additional param here, we'll get it as a compile time constant, and then we
> could store file_line_ as a string piece with a compile-time generated length,
> meaning we never need to run it searching for the null-term in the file_name()
> and line_number() methods. 
> which in turn brings them nearer to the hacker_style() requirement of being
mere
> getters; at the moment they feel rather like they should be FileName() style
> methods.

[joth, 2011-09-15 16:45:22]

On 15 September 2011 17:26, <jar@chromium.org> wrote:

>
>
http://codereview.chromium.**org/7778033/diff/1/base/**tracked.h<http://coder...
>
> File base/tracked.h (right):
>
>
http://codereview.chromium.**org/7778033/diff/1/base/**tracked.h#newcode88<ht...
> base/tracked.h:88: #define FROM_HERE tracked_objects::Location( \
> Can you clarify how this is used? Is it sent up as part of crash dumps?
> If not, then I don't see how we'd justify much of any cost in an
> official build.
>
> On 2011/09/03 01:11:41, jbates wrote:
>
>> On 2011/09/03 00:26:33, jar wrote:
>> > I'm not sure how this happened... but the goal is to have zero perf
>>
> impact
>
>> when
>> > we're not tracking objects, such as in a release build.  Toward that
>>
> end, I
>
>> > historically defined stuff like FROM_HERE (when not tracking) to be
>>
> something
>
>> > with constant strings and values, so there would be no chance the
>>
> compile
>
>> would
>> > waste time/space calculating anything.  In addition, I built the
>>
> constructors
>
>> to
>> > ignore their arguments, further ensuring no possible impact.
>> >
>> > What happened to that plan/philosophy?  Do you know??
>>
>
>  I don't know the history of this code. The use cases I've seen are
>>
> pretty heavy
>
>> weight - creating and enqueuing tasks mostly. FROM_HERE is passing and
>>
> storing
>
>> three pointers (not sure the platform specifics of GetProgramCounter,
>>
> but I'd
>
>> guess it's a simple register access). In comparison to the use cases,
>>
> I don't
>
>> think that's too much overhead for enabling performance analysis in
>>
> our
>
>> release/official builds. We can extract incredibly useful information
>>
> from end
>
>> user traces.
>>
>
>
http://codereview.chromium.**org/7778033/diff/15001/base/**tracked.h<http://c...
> File base/tracked.h (right):
>
> http://codereview.chromium.**org/7778033/diff/15001/base/**
>
tracked.h#newcode91<http://codereview.chromium.org/7778033/diff/15001/base/tracked.h#newcode91>
> base/tracked.h:91: __FILE__ ":" STRINGIZE(__LINE__), __FUNCTION__, \
> I think that sizeof(__FILE__) == sizeof(char*)
>
>
 __FILE__ is substituted by pre-processor so I think sizeof(__FILE__) ==
sizeof("path/to/my/file.cc") ==  19.
my quick experiment agrees. And  arraysize(__FILE__) also does the right
thing.

Alternatively, I understand gcc will happily calculate strlen("literal") for
you at compile time, however it would be rather obscure to rely on that.


I don't think you get what you're hoping for.  You may perchance be
> thinking about:
>
> static array[] = __FILE__;
> then sizeof(array) == strlen(array) I think.
>
>
> On 2011/09/15 10:38:09, joth wrote:
>
>> random musing:
>> if we passed in (sizeof(__FILE__) + sizeof( STRINGIZE(__LINE__) - 1)
>>
> as an
>
>> additional param here, we'll get it as a compile time constant, and
>>
> then we
>
>> could store file_line_ as a string piece with a compile-time generated
>>
> length,
>
>> meaning we never need to run it searching for the null-term in the
>>
> file_name()
>
>> and line_number() methods.
>> which in turn brings them nearer to the hacker_style() requirement of
>>
> being mere
>
>> getters; at the moment they feel rather like they should be FileName()
>>
> style
>
>> methods.
>>
>
>
http://codereview.chromium.**org/7778033/<http://codereview.chromium.org/7778...
>

[jbates, 2011-09-15 16:48:33]

http://codereview.chromium.org/7778033/diff/1/base/tracked.h
File base/tracked.h (right):

http://codereview.chromium.org/7778033/diff/1/base/tracked.h#newcode88
base/tracked.h:88: #define FROM_HERE tracked_objects::Location( \
On 2011/09/15 16:26:39, jar wrote:
> Can you clarify how this is used? Is it sent up as part of crash dumps?  If
not,
> then I don't see how we'd justify much of any cost in an official build.

Now that we have about:tracing in the official build, we have been able to
quickly solve multiple performance bugs reported by end users. Typical scenario:
the user reports a bug, we ask for a trace, and then we analyze the trace.
Without showing all tasks, we may miss a critical reason for a frame hitch. The
cost is 2 pointers in PendingTask. If there were 100s of PendingTasks pending,
it would only be a few KB of additional memory usage.

http://codereview.chromium.org/7778033/diff/15001/base/message_loop.h
File base/message_loop.h (right):

http://codereview.chromium.org/7778033/diff/15001/base/message_loop.h#newcode434
base/message_loop.h:434: tracked_objects::Location posted_from;
On 2011/09/15 10:38:09, joth wrote:
> I managed to avoid this one by using post_births->location() (but, bearing in
> mind post_births maybe NULL esp early in startup)
> 
> This does make it limited to TRACK_ALL_TASK_OBJECTS builds though, but that
> seemed a reasonable trade off?

Are we talking about a memory tradeoff? I actually don't think adding two
pointers to PendingTask significantly effects memory usage. These objects
typically only exist while a task is pending. The advantage of being able to
trace tasks in the field will be invaluable, since we often ask reporters of
bugs to run traces while reproducing a bug.

http://codereview.chromium.org/7778033/diff/15001/base/tracked.cc
File base/tracked.cc (right):

http://codereview.chromium.org/7778033/diff/15001/base/tracked.cc#newcode54
base/tracked.cc:54: return base::StringPiece(function_name_);
On 2011/09/15 10:38:09, joth wrote:
> nit: is there any point returning this as a string piece when the caller just
> wants to call .data() on it? The implicit strlen() call here is then wasted
> effort, and the caller has to make an assumption that data() will be null
> terminated. (c.f. it's easy for the caller to promote char* to StringPiece if
> they need to).

I went back and forth on this one -- I was on the fence because of the lack of
perf-critical use cases, but may as well change to const char*.

http://codereview.chromium.org/7778033/diff/15001/base/tracked.h
File base/tracked.h (right):

http://codereview.chromium.org/7778033/diff/15001/base/tracked.h#newcode91
base/tracked.h:91: __FILE__ ":" STRINGIZE(__LINE__), __FUNCTION__, \
On 2011/09/15 10:38:09, joth wrote:
> random musing:
> if we passed in (sizeof(__FILE__) + sizeof( STRINGIZE(__LINE__) - 1) as an
> additional param here, we'll get it as a compile time constant, and then we
> could store file_line_ as a string piece with a compile-time generated length,
> meaning we never need to run it searching for the null-term in the file_name()
> and line_number() methods. 
> which in turn brings them nearer to the hacker_style() requirement of being
mere
> getters; at the moment they feel rather like they should be FileName() style
> methods.

I think it's best to just change to FileName style - there are no performance
critical usecases for file_name.

[jbates, 2011-09-29 22:59:36]

PTAL.

I removed the file/line merge change from Location and disabled the task trace
events when OFFICIAL_BUILD is defined.

http://codereview.chromium.org/7778033/diff/24001/base/location.h
File base/location.h (left):

http://codereview.chromium.org/7778033/diff/24001/base/location.h#oldcode66
base/location.h:66: const void* const program_counter_;
This change is still require to store a Location in PendingTask objects for
MessageLoop, because PendingTask needs a operator= so it can be used with
std::queue.

http://codereview.chromium.org/7778033/diff/24001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (left):

http://codereview.chromium.org/7778033/diff/24001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:651: from_here.program_counter()));
Cleaner thanks to copyable Location change...

[jar (doing other things), 2011-09-29 23:47:05]

nits below: You may choose to make them a separate CL.

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

http://codereview.chromium.org/7778033/diff/24001/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:685: FROM_HERE,
NewRunnableFunction(&TraceWithAllMacroVariants,
nit: Please run lint on these files.  Indentation when you can't finish a line
should be 4 characters. This is consistently off in this file.

see: 

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

[jbates, 2011-09-30 17:56:05]

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

http://codereview.chromium.org/7778033/diff/24001/base/debug/trace_event_unit...
base/debug/trace_event_unittest.cc:685: FROM_HERE,
NewRunnableFunction(&TraceWithAllMacroVariants,
On 2011/09/29 23:47:05, jar wrote:
> nit: Please run lint on these files.  Indentation when you can't finish a line
> should be 4 characters. This is consistently off in this file.
> 
> see: 
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Done.

[jar (doing other things), 2011-09-30 21:14:48]

LGTM

[darin (slow to review), 2011-09-30 22:23:23]

LGTM

[akalin, 2011-10-03 17:21:55]

sync stuff LGTM