A couple comment updates

A couple comment updates

Add a comment to worker pool, as on first reading it I thought there was a bug with name going out of scope.
+ a minor fix the thread.h

BUG=None
TEST=None


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

[jar (doing other things), 2011-11-04 15:30:32]

Worker threads do come and go, and as as a result it would be a mistake to refer
to them (such as by retaining a pointer to a Thread instance, and using it)
after the thread has been retired.  This is true before or after this proposed
change.

The profiler (about:tracking) code handles this by storing this data (thread
name) in structures held by TLS, and properly handles recycling when the worker
thread is terminated.  I'd suggest that tracing go that route instead (to avoid
holding stale pointers to strings).  [...or hopefully soon, integrate with the
profiler code, and not have to worry about this ;-) ]

I'm somewhat ambivalent to this change.  It is not perf critical... the const
was sort of nice... using an append is surely a tad faster (which doesn't matter
here) than printf... having one copy of the (short) string is a tiny bit
nicer... 

...but when it is all told... it seems like nit-level stuff, without any
detectable upside.

Does this really seem that much better to you, now that you realize the old
version holds the string on the stack?

http://codereview.chromium.org/8463004/diff/1/base/threading/worker_pool_posi...
File base/threading/worker_pool_posix.cc (right):

http://codereview.chromium.org/8463004/diff/1/base/threading/worker_pool_posi...
base/threading/worker_pool_posix.cc:79: name_.reserve(name_.size() + 10);
nit: Reserve is not needed.

[joth, 2011-11-04 16:41:25]

Agreed. I think the biggest benefit (and it's a small one) is consistency with
simple_thread.cc and thread.cc, as I can imagine someone else doing the same as
me in future and thinking it's passing in a temporary to SetName(). I'm happy
either way though.

http://codereview.chromium.org/8463004/diff/1/base/threading/worker_pool_posi...
File base/threading/worker_pool_posix.cc (right):

http://codereview.chromium.org/8463004/diff/1/base/threading/worker_pool_posi...
base/threading/worker_pool_posix.cc:79: name_.reserve(name_.size() + 10);
On 2011/11/04 15:30:32, jar wrote:
> nit: Reserve is not needed.

will remove

[jar (doing other things), 2011-11-04 17:45:59]

With all code changes come risks (even as little as a stupid compiler bug
on a boundary, etc.).

Would adding a comment to the code, clarifying why it is safe to pass the
pointer to the stack allocated std::string be enough to fulfill your gains??

I agree, it is *always* scary when I see a foo.str() being passed out... I
wonder how long the user will use it :-(.

Jim

On Fri, Nov 4, 2011 at 9:41 AM, <joth@chromium.org> wrote:

> Agreed. I think the biggest benefit (and it's a small one) is consistency
> with
> simple_thread.cc and thread.cc, as I can imagine someone else doing the
> same as
> me in future and thinking it's passing in a temporary to SetName(). I'm
> happy
> either way though.
>
>
>
> http://codereview.chromium.**org/8463004/diff/1/base/**
>
threading/worker_pool_posix.cc<http://codereview.chromium.org/8463004/diff/1/base/threading/worker_pool_posix.cc>
> File base/threading/worker_pool_**posix.cc (right):
>
> http://codereview.chromium.**org/8463004/diff/1/base/**
>
threading/worker_pool_posix.**cc#newcode79<http://codereview.chromium.org/8463004/diff/1/base/threading/worker_pool_posix.cc#newcode79>
> base/threading/worker_pool_**posix.cc:79: name_.reserve(name_.size() +
> 10);
> On 2011/11/04 15:30:32, jar wrote:
>
>> nit: Reserve is not needed.
>>
>
> will remove
>
>
http://codereview.chromium.**org/8463004/<http://codereview.chromium.org/8463...
>

[joth, 2011-11-04 18:23:09]

Combining threads...

On 4 November 2011 15:30, <jar@chromium.org> wrote:

> Worker threads do come and go, and as as a result it would be a mistake to
> refer
> to them (such as by retaining a pointer to a Thread instance, and using it)
> after the thread has been retired.  This is true before or after this
> proposed
> change.
>
> The profiler (about:tracking) code handles this by storing this data
> (thread
> name) in structures held by TLS, and properly handles recycling when the
> worker
> thread is terminated.  I'd suggest that tracing go that route instead (to
> avoid
> holding stale pointers to strings).  [...or hopefully soon, integrate with
> the
> profiler code, and not have to worry about this ;-) ]
>
>

There isn't any problem with trace event holding name pointers after the
thread dies: it takes a full copy of the name only when running in the
context of the thread in question, hence the source char* is sure to still
be valid.
I did find the source of my bug though, which was it wasn't correctly
handling threads that change name, as it would only record each thread's
name once, and it may not yet have been set on startup.
Fixed in http://codereview.chromium.org/8470001/

Agree it would be good to converge the two approaches. With small benefit
with the current trace_event approach is that it gets the 'real' names for
all platform threads (worker, simple etc) without needing any hooks in each
of them, or having to fall back to synthesizing names for them.



On 4 November 2011 17:45, Jim Roskind <jar@chromium.org> wrote:

> With all code changes come risks (even as little as a stupid compiler bug
> on a boundary, etc.).
>
> Would adding a comment to the code, clarifying why it is safe to pass the
> pointer to the stack allocated std::string be enough to fulfill your gains??
>
> I agree, it is *always* scary when I see a foo.str() being passed out... I
> wonder how long the user will use it :-(.
>
>
yeah, a comment fix up seems fine.
(although it does just mildly irk me that PosixDynamicThreadPool *and*
each WorkerThread instance are making a copy heap copy of the name_prefix_
string that could just be a compile time char* constant shared across the
lot of them.....)

[joth, 2011-11-07 11:22:30]

On 2011/11/04 17:45:59, jar wrote:
> With all code changes come risks (even as little as a stupid compiler bug
> on a boundary, etc.).
> 
> Would adding a comment to the code, clarifying why it is safe to pass the
> pointer to the stack allocated std::string be enough to fulfill your gains??
> 
Changed to a comment-only patch.
PTAL

Thanks.


> I agree, it is *always* scary when I see a foo.str() being passed out... I
> wonder how long the user will use it :-(.
> 
> Jim
> 
> On Fri, Nov 4, 2011 at 9:41 AM, <mailto:joth@chromium.org> wrote:
> 
> > Agreed. I think the biggest benefit (and it's a small one) is consistency
> > with
> > simple_thread.cc and thread.cc, as I can imagine someone else doing the
> > same as
> > me in future and thinking it's passing in a temporary to SetName(). I'm
> > happy
> > either way though.
> >
> >
> >
> > http://codereview.chromium.**org/8463004/diff/1/base/**
> >
>
threading/worker_pool_posix.cc<http://codereview.chromium.org/8463004/diff/1/base/threading/worker_pool_posix.cc>
> > File base/threading/worker_pool_**posix.cc (right):
> >
> > http://codereview.chromium.**org/8463004/diff/1/base/**
> >
>
threading/worker_pool_posix.**cc#newcode79<http://codereview.chromium.org/8463004/diff/1/base/threading/worker_pool_posix.cc#newcode79>
> > base/threading/worker_pool_**posix.cc:79: name_.reserve(name_.size() +
> > 10);
> > On 2011/11/04 15:30:32, jar wrote:
> >
> >> nit: Reserve is not needed.
> >>
> >
> > will remove
> >
> >
>
http://codereview.chromium.**org/8463004/%3Chttp://codereview.chromium.org/84...>
> >

[jar (doing other things), 2011-11-07 16:32:48]

lgtm

[commit-bot: I haz the power, 2011-11-07 16:32:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/8463004/6001

[commit-bot: I haz the power, 2011-11-07 18:07:49]

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

[commit-bot: I haz the power, 2011-11-08 09:08:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/8463004/6001

[commit-bot: I haz the power, 2011-11-08 10:27:19]

Change committed as 109002