Add ability to retrieve a thread_name given a thread_id.

Add ability to retrieve a thread_name given a thread_id.

The work to add GPU tracing needs the ability to map from a thread_id back
to a thread_name in order to allow us to have the correct display on the
chrome://tracing page.

We want to be able to output our traces without having to post to a thread
to handle the output, and to have the GPU traces differentiated from the
GPU-process which will be doing the tracing calls.

BUG=111509
TEST=Added tests in base_unittests


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178886

[jonathan.backer, 2012-12-05 20:06:12]

https://codereview.chromium.org/11438022/diff/3001/base/threading/thread_id_n...
File base/threading/thread_id_name_manager.h (right):

https://codereview.chromium.org/11438022/diff/3001/base/threading/thread_id_n...
base/threading/thread_id_name_manager.h:16: template <typename T> struct
DefaultSingletonTraits;
Different traits define kAllowedToAccessOnNonjoinableThread differently. I'm not
sure which one you want here.

https://codereview.chromium.org/11438022/diff/3001/base/threading/thread_id_n...
File base/threading/thread_id_name_manager_unittest.cc (right):

https://codereview.chromium.org/11438022/diff/3001/base/threading/thread_id_n...
base/threading/thread_id_name_manager_unittest.cc:39:
EXPECT_FALSE(manager->GetNameForId(1234, &name));
How do you know that this isn't a valid thread id?

[dsinclair, 2012-12-05 20:40:09]

https://codereview.chromium.org/11438022/diff/3001/base/threading/thread_id_n...
File base/threading/thread_id_name_manager.h (right):

https://codereview.chromium.org/11438022/diff/3001/base/threading/thread_id_n...
base/threading/thread_id_name_manager.h:16: template <typename T> struct
DefaultSingletonTraits;
On 2012/12/05 20:06:12, jonathan.backer wrote:
> Different traits define kAllowedToAccessOnNonjoinableThread differently. I'm
not
> sure which one you want here.

Switched to LeakySingletonTraits.

https://codereview.chromium.org/11438022/diff/3001/base/threading/thread_id_n...
File base/threading/thread_id_name_manager_unittest.cc (right):

https://codereview.chromium.org/11438022/diff/3001/base/threading/thread_id_n...
base/threading/thread_id_name_manager_unittest.cc:39:
EXPECT_FALSE(manager->GetNameForId(1234, &name));
On 2012/12/05 20:06:12, jonathan.backer wrote:
> How do you know that this isn't a valid thread id?

Good point, removed the test.

[dsinclair, 2012-12-06 16:08:33]

jar@ can you please review these changes in base/.

They are needed to support https://codereview.chromium.org/11366109/ which, in
turn, is needed for https://codereview.chromium.org/11416117/.

Thanks,
dan

[jonathan.backer, 2012-12-06 16:29:08]

https://codereview.chromium.org/11438022/diff/13001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/11438022/diff/13001/base/base.gyp#newcode545
base/base.gyp:545: 'threading/thread_id_name_manager_unittest.cc',
nit: sorted order?

https://codereview.chromium.org/11438022/diff/13001/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/11438022/diff/13001/base/base.gypi#newcode445
base/base.gypi:445: 'threading/thread_id_name_manager.h',
nit: alphabetical order?

https://codereview.chromium.org/11438022/diff/13001/base/threading/platform_t...
File base/threading/platform_thread_mac.mm (right):

https://codereview.chromium.org/11438022/diff/13001/base/threading/platform_t...
base/threading/platform_thread_mac.mm:24: current_thread_name =
LAZY_INSTANCE_INITIALIZER;
Why the duplicated state? Now we have TLS for the pointer, but also in the
std::map.

[dsinclair, 2012-12-06 17:55:05]

https://codereview.chromium.org/11438022/diff/13001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/11438022/diff/13001/base/base.gyp#newcode545
base/base.gyp:545: 'threading/thread_id_name_manager_unittest.cc',
On 2012/12/06 16:29:08, jonathan.backer wrote:
> nit: sorted order?

Done.

https://codereview.chromium.org/11438022/diff/13001/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/11438022/diff/13001/base/base.gypi#newcode445
base/base.gypi:445: 'threading/thread_id_name_manager.h',
On 2012/12/06 16:29:08, jonathan.backer wrote:
> nit: alphabetical order?

Done.

https://codereview.chromium.org/11438022/diff/13001/base/threading/platform_t...
File base/threading/platform_thread_mac.mm (right):

https://codereview.chromium.org/11438022/diff/13001/base/threading/platform_t...
base/threading/platform_thread_mac.mm:24: current_thread_name =
LAZY_INSTANCE_INITIALIZER;
On 2012/12/06 16:29:08, jonathan.backer wrote:
> Why the duplicated state? Now we have TLS for the pointer, but also in the
> std::map.

Done.

Removed the thread locals from each of the files. Was able to remove the mac
specific PlatformThread::GetName as it becomes equivalent to the posix
implementation.

[jar (doing other things), 2012-12-06 20:54:37]

I had a couple of concerns that I'd like to hear comments on:

a) Why don't you strdup the string name?  ... or equivalently, how are you
assured that you'll drop the pointer when the storage is discarded?  I *think*
you may be ok, but the fact that you're at arms length from the thread instance
(and can be called from anyone anywhere to set a mapping), makes me wary of the
fragility.

b) Perf angle: I can't think of who would be calling for the thread name very
often, but you've changed the cost from a TLS lookup, to a TLS lookup (to get
thread ID) plus a locked access to the map.  I'd expect this to only be called
rarely, or by the debugger, but I'm curious about who calls for the name and
when.  Have you put a breakpoint in, and verified it is called for very rarely?

Thanks.

https://codereview.chromium.org/11438022/diff/13001/base/threading/thread_id_...
File base/threading/thread_id_name_manager.cc (right):

https://codereview.chromium.org/11438022/diff/13001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:34: id_to_name_[id] = name;
nit: can/should we have a DCHECK here to verify that it is not set twice?

https://codereview.chromium.org/11438022/diff/13001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:39: id_to_name_.erase(id);
nit: can/should we have a DCHECK here to verify that it was indeed set
previously (i.e., that we didn't miss a hook to set it?)  Or are there worker
threads that just don't get set?

https://codereview.chromium.org/11438022/diff/13001/base/threading/thread_id_...
File base/threading/thread_id_name_manager_unittest.cc (right):

https://codereview.chromium.org/11438022/diff/13001/base/threading/thread_id_...
base/threading/thread_id_name_manager_unittest.cc:21: base::Thread a(kAThread);
nit: avoid such use of single letter names.  Better would be thread_a or such.

[dsinclair, 2012-12-10 16:24:36]

PTAL.

I've updated to strdup the names into the manager. I don't see anything that
requires those names to static data.

The only caller of PlatformThread::GetName is the trace event framework. In
terms of Thread::thread_name() there are only a couple of calls in the code.
When a BrowserThreadImpl is created, when SyncScheduleImpl is started and when a
TrackedObject::BirthOnThreadSnapshot is called.

https://codereview.chromium.org/11438022/diff/13001/base/threading/thread_id_...
File base/threading/thread_id_name_manager.cc (right):

https://codereview.chromium.org/11438022/diff/13001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:34: id_to_name_[id] = name;
On 2012/12/06 20:54:37, jar wrote:
> nit: can/should we have a DCHECK here to verify that it is not set twice?

It seems that being able to SetName multiple times is expected behaviour. There
is a unittest in the trace_event code which tests that exact thing.

https://codereview.chromium.org/11438022/diff/13001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:39: id_to_name_.erase(id);
On 2012/12/06 20:54:37, jar wrote:
> nit: can/should we have a DCHECK here to verify that it was indeed set
> previously (i.e., that we didn't miss a hook to set it?)  Or are there worker
> threads that just don't get set?

It's possible the name wouldn't be set, but in that case we'd also have a
kInvalidThreadId pass for id. So, I can handle that case and add the DCHECK.

https://codereview.chromium.org/11438022/diff/13001/base/threading/thread_id_...
File base/threading/thread_id_name_manager_unittest.cc (right):

https://codereview.chromium.org/11438022/diff/13001/base/threading/thread_id_...
base/threading/thread_id_name_manager_unittest.cc:21: base::Thread a(kAThread);
On 2012/12/06 20:54:37, jar wrote:
> nit: avoid such use of single letter names.  Better would be thread_a or such.

Done.

[dsinclair, 2012-12-13 17:43:42]

I've added increasing version numbers to go along with the thread names. The
reason for this is that in the trace framework there is the expectation that a
call to SetName will usually produce a different memory address then previous
calls. But, with the strdup/free there is a greater possibility that we'll get
the same memory allocated to the newly allocated name.

So, I've added the version number for the thread name that can be stored, then
we can just compare the previous version number to the current thread version
number to determine if the name has changed.

[jar (doing other things), 2012-12-14 18:23:07]

https://codereview.chromium.org/11438022/diff/33001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/11438022/diff/33001/base/base.gyp#newcode327
base/base.gyp:327: 'prefs/pref_observer.h',
nit: alphabetize

https://codereview.chromium.org/11438022/diff/33001/base/debug/trace_event_im...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/11438022/diff/33001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:67: // The most-recently captured name version of
the current thread
Please explain what a "name version" is for a thread.

https://codereview.chromium.org/11438022/diff/33001/base/threading/thread_id_...
File base/threading/thread_id_name_manager.cc (right):

https://codereview.chromium.org/11438022/diff/33001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:30: return id_to_name_[id];
I have a tiny concern about the lifetime of this character buffer.  Once the
lock is released, it could be deleted asynchronously. 

How often is this used?  Any chance a copy, such as via std:string, would be
cleaner or safer?  Am I being too paranoid here?

[dsinclair, 2012-12-14 19:13:46]

https://codereview.chromium.org/11438022/diff/33001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/11438022/diff/33001/base/base.gyp#newcode327
base/base.gyp:327: 'prefs/pref_observer.h',
On 2012/12/14 18:23:07, jar wrote:
> nit: alphabetize

Done.

https://codereview.chromium.org/11438022/diff/33001/base/debug/trace_event_im...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/11438022/diff/33001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:67: // The most-recently captured name version of
the current thread
On 2012/12/14 18:23:07, jar wrote:
> Please explain what a "name version" is for a thread.

Done.

https://codereview.chromium.org/11438022/diff/33001/base/threading/thread_id_...
File base/threading/thread_id_name_manager.cc (right):

https://codereview.chromium.org/11438022/diff/33001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:30: return id_to_name_[id];
On 2012/12/14 18:23:07, jar wrote:
> I have a tiny concern about the lifetime of this character buffer.  Once the
> lock is released, it could be deleted asynchronously. 
> 
> How often is this used?  Any chance a copy, such as via std:string, would be
> cleaner or safer?  Am I being too paranoid here?

Currently, this is used in one place, the tracing framework code.

As far as I can tell from searching around, std::string isn't necessarily thread
safe either. I think the only way we'd be safe is to strdup and then make the
caller free which is getting ugly.

The other thing to keep in mind here is, we want this as fast as possible. The
tracing code maybe calling this a lot, although I can cut down that number now I
believe.

So, I'm not sure the correct solution for this, and am happy to take
suggestions.

[jar (doing other things), 2012-12-14 19:58:08]

https://codereview.chromium.org/11438022/diff/33001/base/threading/thread_id_...
File base/threading/thread_id_name_manager.cc (right):

https://codereview.chromium.org/11438022/diff/33001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:30: return id_to_name_[id];
Sorry for my seemingly confusing mention of std::string.

I was suggesting returning a std::string temporary, which was effectively an
allocated copy of the char*.  We can probably set this up so that the caller
does not explicitly call delete, but rather the temporary is discarded (in due
course) after use by the caller.

That does have a cost that involves a call to malloc (implicitly).  We do also
have a stack-allocated-string, which could allow slightly less cost... but there
is always the cost of the memcpy action :-/.

It could also be that this code is only significantly active when a person is
debugging, and hence we can afford the risk... or it could be that threads
should not go away while doing this debugging... and that is our safe haven.  I
wanted to at least talk about it.

You could also just leak the char*, rather than deleting it with the
destruction.  Assuming that threads are not created and destroyed regularly (and
named), this would be a non-problem.  Do you know if we have a lot of creation
and destruction of threads (and names)?  I didn't think we did :-/.

On 2012/12/14 19:13:46, dsinclair wrote:
> On 2012/12/14 18:23:07, jar wrote:
> > I have a tiny concern about the lifetime of this character buffer.  Once the
> > lock is released, it could be deleted asynchronously. 
> > 
> > How often is this used?  Any chance a copy, such as via std:string, would be
> > cleaner or safer?  Am I being too paranoid here?
> 
> Currently, this is used in one place, the tracing framework code.
> 
> As far as I can tell from searching around, std::string isn't necessarily
thread
> safe either. I think the only way we'd be safe is to strdup and then make the
> caller free which is getting ugly.
> 
> The other thing to keep in mind here is, we want this as fast as possible. The
> tracing code maybe calling this a lot, although I can cut down that number now
I
> believe.
> 
> So, I'm not sure the correct solution for this, and am happy to take
> suggestions.

[dsinclair, 2012-12-14 20:18:19]

https://codereview.chromium.org/11438022/diff/33001/base/threading/thread_id_...
File base/threading/thread_id_name_manager.cc (right):

https://codereview.chromium.org/11438022/diff/33001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:30: return id_to_name_[id];
On 2012/12/14 19:58:08, jar wrote:
> I was suggesting returning a std::string temporary, which was effectively an
> allocated copy of the char*.  We can probably set this up so that the caller
> does not explicitly call delete, but rather the temporary is discarded (in due
> course) after use by the caller.

Sorry, I was thinking the copy-on-write semantics of the std::string would kick
in but the creation from a char* would be an alloc/copy.


> It could also be that this code is only significantly active when a person is
> debugging, and hence we can afford the risk... or it could be that threads
> should not go away while doing this debugging... and that is our safe haven. 
I
> wanted to at least talk about it.

I don't know about in the future, but currently this would only be accessed when
you're using chrome://tracing. The trace framework has logic so the calls won't
be made unless tracing is currently enabled.

 
> You could also just leak the char*, rather than deleting it with the
> destruction.  Assuming that threads are not created and destroyed regularly
(and
> named), this would be a non-problem.  Do you know if we have a lot of creation
> and destruction of threads (and names)?  I didn't think we did :-/.

The name is part of the constructor for a thread, so they should always be
named. I don't know what the rate of creation is for threads in the system.

[dsinclair, 2012-12-20 20:00:42]

ping.



On 2012/12/14 20:18:19, dsinclair wrote:
>
https://codereview.chromium.org/11438022/diff/33001/base/threading/thread_id_...
> File base/threading/thread_id_name_manager.cc (right):
> 
>
https://codereview.chromium.org/11438022/diff/33001/base/threading/thread_id_...
> base/threading/thread_id_name_manager.cc:30: return id_to_name_[id];
> On 2012/12/14 19:58:08, jar wrote:
> > I was suggesting returning a std::string temporary, which was effectively an
> > allocated copy of the char*.  We can probably set this up so that the caller
> > does not explicitly call delete, but rather the temporary is discarded (in
due
> > course) after use by the caller.
> 
> Sorry, I was thinking the copy-on-write semantics of the std::string would
kick
> in but the creation from a char* would be an alloc/copy.
> 
> 
> > It could also be that this code is only significantly active when a person
is
> > debugging, and hence we can afford the risk... or it could be that threads
> > should not go away while doing this debugging... and that is our safe haven.

> I
> > wanted to at least talk about it.
> 
> I don't know about in the future, but currently this would only be accessed
when
> you're using chrome://tracing. The trace framework has logic so the calls
won't
> be made unless tracing is currently enabled.
> 
>  
> > You could also just leak the char*, rather than deleting it with the
> > destruction.  Assuming that threads are not created and destroyed regularly
> (and
> > named), this would be a non-problem.  Do you know if we have a lot of
creation
> > and destruction of threads (and names)?  I didn't think we did :-/.
> 
> The name is part of the constructor for a thread, so they should always be
> named. I don't know what the rate of creation is for threads in the system.

[jar (doing other things), 2012-12-21 00:16:59]

https://codereview.chromium.org/11438022/diff/33001/base/debug/trace_event_im...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/11438022/diff/33001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:67: // The most-recently captured name version of
the current thread
The comment change was helpful, as I now am able to parse that this is the
"version number" that is associated with a "thread name.".

I don't grok why you have a version number at all however.  Can you add that to
the comment?

On 2012/12/14 19:13:46, dsinclair wrote:
> On 2012/12/14 18:23:07, jar wrote:
> > Please explain what a "name version" is for a thread.
> 
> Done.

https://codereview.chromium.org/11438022/diff/38003/base/debug/trace_event_un...
File base/debug/trace_event_unittest.cc (right):

https://codereview.chromium.org/11438022/diff/38003/base/debug/trace_event_un...
base/debug/trace_event_unittest.cc:90: old_thread_name_ = NULL;
nit: Using the fact that you can free a NULL pointer with no ill effects, you
could have just added line 89 and 90 to the old code.  I think it would be
easier to read.  Resulting code would read:
PlatformThread::SetName(old_thread_name_ ? old_thread_name_  : "");
free(old_thread_name_);
old_thread_name_ = NULL;

https://codereview.chromium.org/11438022/diff/38003/base/threading/thread_id_...
File base/threading/thread_id_name_manager.cc (right):

https://codereview.chromium.org/11438022/diff/38003/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:38: id_to_name_[id] = strdup(name);
It is always good form to do as little work inside a lock as possible.  Hence
better would be:
a) strdup the argument
b) get lock
c) extract current name if any, into a temp variable.
d) set current name to the result of (a)
e) release lock
f) free result of (c).

https://codereview.chromium.org/11438022/diff/38003/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:52: free(old_name);
Here again, you can do the free after you release the lock

[dsinclair, 2012-12-21 16:28:59]

@jar PTAL.

https://codereview.chromium.org/11438022/diff/33001/base/debug/trace_event_im...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/11438022/diff/33001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:67: // The most-recently captured name version of
the current thread
On 2012/12/21 00:16:59, jar wrote:
> The comment change was helpful, as I now am able to parse that this is the
> "version number" that is associated with a "thread name.".
> 
> I don't grok why you have a version number at all however.  Can you add that
to
> the comment?
> 
> On 2012/12/14 19:13:46, dsinclair wrote:
> > On 2012/12/14 18:23:07, jar wrote:
> > > Please explain what a "name version" is for a thread.
> > 
> > Done.
> 

Done.

https://codereview.chromium.org/11438022/diff/38003/base/debug/trace_event_un...
File base/debug/trace_event_unittest.cc (right):

https://codereview.chromium.org/11438022/diff/38003/base/debug/trace_event_un...
base/debug/trace_event_unittest.cc:90: old_thread_name_ = NULL;
On 2012/12/21 00:17:00, jar wrote:
> nit: Using the fact that you can free a NULL pointer with no ill effects, you
> could have just added line 89 and 90 to the old code.  I think it would be
> easier to read.  Resulting code would read:
> PlatformThread::SetName(old_thread_name_ ? old_thread_name_  : "");
> free(old_thread_name_);
> old_thread_name_ = NULL;

Done.

That's a lot nicer, didn't realize free(NULL) was a no-op.

https://codereview.chromium.org/11438022/diff/38003/base/threading/thread_id_...
File base/threading/thread_id_name_manager.cc (right):

https://codereview.chromium.org/11438022/diff/38003/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:38: id_to_name_[id] = strdup(name);
On 2012/12/21 00:17:00, jar wrote:
> It is always good form to do as little work inside a lock as possible.  Hence
> better would be:
> a) strdup the argument
> b) get lock
> c) extract current name if any, into a temp variable.
> d) set current name to the result of (a)
> e) release lock
> f) free result of (c).

Done.

https://codereview.chromium.org/11438022/diff/38003/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:52: free(old_name);
On 2012/12/21 00:17:00, jar wrote:
> Here again, you can do the free after you release the lock

Done.

[jar (doing other things), 2012-12-26 22:41:11]

+cc nduca for a clearer use case.

https://codereview.chromium.org/11438022/diff/55001/base/debug/trace_event_im...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/11438022/diff/55001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:71: // as ThreadIdNameManager has a higher chance
of giving us the same memory
I still don't get the use case.
I think you're saying that you can't use the optimization of treating the
pointer to the original string as an atom, since the string might be
deallocated/reallocated.  

I can understand that....

so maybe I could see having a unique id for the thread (that is not dependent on
name, or where name is stored).  I wouldn't call that a "version of a name," as
that sounds like a single name has many incarnations (versions).

[dsinclair, 2013-01-02 15:45:02]

On 2012/12/26 22:41:11, jar wrote:
> +cc nduca for a clearer use case.
> 
>
https://codereview.chromium.org/11438022/diff/55001/base/debug/trace_event_im...
> File base/debug/trace_event_impl.cc (right):
> 
>
https://codereview.chromium.org/11438022/diff/55001/base/debug/trace_event_im...
> base/debug/trace_event_impl.cc:71: // as ThreadIdNameManager has a higher
chance
> of giving us the same memory
> I still don't get the use case.
> I think you're saying that you can't use the optimization of treating the
> pointer to the original string as an atom, since the string might be
> deallocated/reallocated.  
> 
> I can understand that....
> 
> so maybe I could see having a unique id for the thread (that is not dependent
on
> name, or where name is stored).  I wouldn't call that a "version of a name,"
as
> that sounds like a single name has many incarnations (versions).


By calling it name version I was trying to signify that the version ID would
change when the name of the thread changes. It's the changing of the name that
I'm specifically tracing with the version number. The number won't uniquely
identify a thread as the ID can change during program execution.

[dsinclair, 2013-01-03 21:57:06]

Nat, any comments that can help clear up the confusion here?

Thanks,
dan

[nduca, 2013-01-04 22:30:38]

How about making some helper functions in thread.cc inside an anon namespace a
la:
  typedef InternedString int;
  InternedString internString(const std::string& str);
  const std::string& getInternedStringValue(InternedString);

And then use those as needed...

[dsinclair, 2013-01-07 16:22:27]

On 2013/01/04 22:30:38, nduca wrote:
> How about making some helper functions in thread.cc inside an anon namespace a
> la:
>   typedef InternedString int;
>   InternedString internString(const std::string& str);
>   const std::string& getInternedStringValue(InternedString);
> 
> And then use those as needed...

@nduca, @jar: PTAL.

I switched to an interned name system. The interning is done in
ThreadIdNameManager instead of Thread as I don't have access to the Thread just
the thread_id.

Does this seem like it makes more sense then the version number stuff
previously?

[dsinclair, 2013-01-09 21:08:37]

On 2013/01/07 16:22:27, dsinclair wrote:
> On 2013/01/04 22:30:38, nduca wrote:
> > How about making some helper functions in thread.cc inside an anon namespace
a
> > la:
> >   typedef InternedString int;
> >   InternedString internString(const std::string& str);
> >   const std::string& getInternedStringValue(InternedString);
> > 
> > And then use those as needed...
> 
> @nduca, @jar: PTAL.
> 
> I switched to an interned name system. The interning is done in
> ThreadIdNameManager instead of Thread as I don't have access to the Thread
just
> the thread_id.
> 
> Does this seem like it makes more sense then the version number stuff
> previously?

ping @nduca, @jar

[jar (doing other things), 2013-01-14 19:27:57]

I just started to look at the CL.... but could need a better understanding
before going beyond the question below.

https://codereview.chromium.org/11438022/diff/81001/base/debug/trace_event_im...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/11438022/diff/81001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:71: base::InternedString
g_current_thread_interned_name =
I was expecting such a thing to be in TLS, and it's not clear how this is held
in a global.

https://codereview.chromium.org/11438022/diff/81001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:647: if (interned_name !=
g_current_thread_interned_name) {
How does this work without racing access to the global?

[dsinclair, 2013-01-14 22:26:09]

https://codereview.chromium.org/11438022/diff/81001/base/debug/trace_event_im...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/11438022/diff/81001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:71: base::InternedString
g_current_thread_interned_name =
On 2013/01/14 19:27:57, jar wrote:
> I was expecting such a thing to be in TLS, and it's not clear how this is held
> in a global.

Thanks for pointing that out, lost the ThreadLocal at some point when I was
working on this. It's back now.

https://codereview.chromium.org/11438022/diff/81001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:647: if (interned_name !=
g_current_thread_interned_name) {
On 2013/01/14 19:27:57, jar wrote:
> How does this work without racing access to the global?

Moved the storage to a thread local. There is an AutoLock lock around this
specific region of code, so should fix any races.

[jar (doing other things), 2013-01-17 23:18:38]

https://codereview.chromium.org/11438022/diff/81001/base/debug/trace_event_im...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/11438022/diff/81001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:647: if (interned_name !=
g_current_thread_interned_name) {
Shouldn't there be a test that would break if we didn't have TLS?  Can you add
one?

On 2013/01/14 22:26:09, dsinclair wrote:
> On 2013/01/14 19:27:57, jar wrote:
> > How does this work without racing access to the global?
> 
> Moved the storage to a thread local. There is an AutoLock lock around this
> specific region of code, so should fix any races.

https://codereview.chromium.org/11438022/diff/87001/base/debug/trace_event_im...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/11438022/diff/87001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:642: InternedString interned_name =
manager->GetInternedName(thread_id);
nit: declare/define vars as close as possible to first use.  Move lines 640-642
down to in front of line 652 (or equivalently, move your new block 645-647 up to
around line 639).

https://codereview.chromium.org/11438022/diff/87001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:645: if
(!g_current_thread_interned_name.Get().Get())
nit: not sure about perf impact... but I suspect you should put much if not all
of the above into a local for use in line 652 (and maybe 655).  You're going
through a singleton (locking)... then finding the TLS map.... then getting a
value.  Its seems sad to do this too many times

https://codereview.chromium.org/11438022/diff/87001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:647: new
InternedString(base::kDefaultInternedString));
nit: use curlies around any if when you span more than one line.

https://codereview.chromium.org/11438022/diff/87001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:654: if (new_name && *new_name) {
See comments about GetInternedStringValue().  I don't think you want to define
it to return NULL.... and if so... we can avoid checking for it.

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
File base/threading/thread_id_name_manager.cc (right):

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:16: const base::InternedString
kDefaultInternedString = 0;
nit: avoid using base:: thoughout, since we're in the base namespace.

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:32: {
nit: curlies not needed, since we return when we release the lock.

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:41: id_to_interned_name_[id] =
interned_id;
nit: code might be cleaner if line 41 and line 36 were only written once. 
Change the sense of the if on line 35 and this will be a more standard pattern:
if (!in cache)
  cache it;
return cached value;

That pattern also has the advantage that the fast (cached) path does one lookup.
 (not sure if perf is an issue here... but it is possibly a nicer pattern).

Similar pattern would apply to line 63 and 76.

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:55: {
nit: remove curlies.

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:67: if
(!id_to_interned_name_.count(id))
nit: a consistently better pattern is probably to use .find().  If the iterator
is .end(), then it is missing, otherwise you can just return *it.

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:77: return NULL;
At first I thought this was a result of a very invalid argument.  Then I saw
that you get this when you pass in the kDefaultInternedString.

I think the code would be cleaner if you actually had a string for this interned
id of 0, and we never had such error conditions (and when we do, we *know* it is
an error).

As it stands now, such errors get bundled into this line, and not detected.

You could have a call to a static GetDefaultInternedString() which created a ""
string and interned it, and not publicize the constant 0 for instance.

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
File base/threading/thread_id_name_manager.h (right):

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
base/threading/thread_id_name_manager.h:25: 
nit: remove blank line

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
base/threading/thread_id_name_manager.h:32: const char*
GetNameForId(PlatformThreadId id);
Why  do we have a getter that gets the char* for the thread, as well as one that
gets the id for a thread, and a getter for the char* from the id?

Should some of these be private?  

Also keep in mind that you'll probably leak your interned strings... so their
address (or the address of their enclosed data) is very permanently unique (even
without and integer id).

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
base/threading/thread_id_name_manager.h:52: std::map<PlatformThreadId,
base::InternedString> id_to_interned_name_;
nit: remove "base::"

do this througout this header, since we're in the base namespace.

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
base/threading/thread_id_name_manager.h:54: std::map<std::string,
base::InternedString> name_to_interned_name_;
nit: remove "base::"

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
base/threading/thread_id_name_manager.h:56: base::InternedString
current_interned_name_;
Why not use something like interned_name_to_name_.size(), rather than having
this redundant counter?

If you really like the counter, a better name would be:
next_unused_interned_id_

[dsinclair, 2013-01-21 19:14:18]

https://codereview.chromium.org/11438022/diff/81001/base/debug/trace_event_im...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/11438022/diff/81001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:647: if (interned_name !=
g_current_thread_interned_name) {
I'm not exactly sure how to write that test. Any suggestions?

This code is actually back to the form it was in prior to me making any changes.
With the other changes suggested I was able to do the name manager without
modifying the trace code.


On 2013/01/17 23:18:39, jar wrote:
> Shouldn't there be a test that would break if we didn't have TLS?  Can you add
> one?
> 
> On 2013/01/14 22:26:09, dsinclair wrote:
> > On 2013/01/14 19:27:57, jar wrote:
> > > How does this work without racing access to the global?
> > 
> > Moved the storage to a thread local. There is an AutoLock lock around this
> > specific region of code, so should fix any races.
>

https://codereview.chromium.org/11438022/diff/87001/base/debug/trace_event_im...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/11438022/diff/87001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:642: InternedString interned_name =
manager->GetInternedName(thread_id);
On 2013/01/17 23:18:39, jar wrote:
> nit: declare/define vars as close as possible to first use.  Move lines
640-642
> down to in front of line 652 (or equivalently, move your new block 645-647 up
to
> around line 639).

Done.

https://codereview.chromium.org/11438022/diff/87001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:645: if
(!g_current_thread_interned_name.Get().Get())
On 2013/01/17 23:18:39, jar wrote:
> nit: not sure about perf impact... but I suspect you should put much if not
all
> of the above into a local for use in line 652 (and maybe 655).  You're going
> through a singleton (locking)... then finding the TLS map.... then getting a
> value.  Its seems sad to do this too many times

Done.

https://codereview.chromium.org/11438022/diff/87001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:647: new
InternedString(base::kDefaultInternedString));
On 2013/01/17 23:18:39, jar wrote:
> nit: use curlies around any if when you span more than one line.

Done.

https://codereview.chromium.org/11438022/diff/87001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:654: if (new_name && *new_name) {
On 2013/01/17 23:18:39, jar wrote:
> See comments about GetInternedStringValue().  I don't think you want to define
> it to return NULL.... and if so... we can avoid checking for it.

Done.

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
File base/threading/thread_id_name_manager.cc (right):

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:16: const base::InternedString
kDefaultInternedString = 0;
On 2013/01/17 23:18:39, jar wrote:
> nit: avoid using base:: thoughout, since we're in the base namespace.

Done.

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:32: {
On 2013/01/17 23:18:39, jar wrote:
> nit: curlies not needed, since we return when we release the lock.

Done.

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:41: id_to_interned_name_[id] =
interned_id;
On 2013/01/17 23:18:39, jar wrote:
> nit: code might be cleaner if line 41 and line 36 were only written once. 
> Change the sense of the if on line 35 and this will be a more standard
pattern:
> if (!in cache)
>   cache it;
> return cached value;
> 
> That pattern also has the advantage that the fast (cached) path does one
lookup.
>  (not sure if perf is an issue here... but it is possibly a nicer pattern).
> 
> Similar pattern would apply to line 63 and 76.

Done.

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:55: {
On 2013/01/17 23:18:39, jar wrote:
> nit: remove curlies.

Done.

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:67: if
(!id_to_interned_name_.count(id))
On 2013/01/17 23:18:39, jar wrote:
> nit: a consistently better pattern is probably to use .find().  If the
iterator
> is .end(), then it is missing, otherwise you can just return *it.

Done.

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
base/threading/thread_id_name_manager.cc:77: return NULL;
On 2013/01/17 23:18:39, jar wrote:
> At first I thought this was a result of a very invalid argument.  Then I saw
> that you get this when you pass in the kDefaultInternedString.
> 
> I think the code would be cleaner if you actually had a string for this
interned
> id of 0, and we never had such error conditions (and when we do, we *know* it
is
> an error).
> 
> As it stands now, such errors get bundled into this line, and not detected.
> 
> You could have a call to a static GetDefaultInternedString() which created a
""
> string and interned it, and not publicize the constant 0 for instance.

Done.

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
File base/threading/thread_id_name_manager.h (right):

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
base/threading/thread_id_name_manager.h:25: 
On 2013/01/17 23:18:39, jar wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
base/threading/thread_id_name_manager.h:32: const char*
GetNameForId(PlatformThreadId id);
Done.

Lost sight of the forest because of the trees.


On 2013/01/17 23:18:39, jar wrote:
> Why  do we have a getter that gets the char* for the thread, as well as one
that
> gets the id for a thread, and a getter for the char* from the id?
> 
> Should some of these be private?  
> 
> Also keep in mind that you'll probably leak your interned strings... so their
> address (or the address of their enclosed data) is very permanently unique
(even
> without and integer id).

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
base/threading/thread_id_name_manager.h:52: std::map<PlatformThreadId,
base::InternedString> id_to_interned_name_;
On 2013/01/17 23:18:39, jar wrote:
> nit: remove "base::"
> 
> do this througout this header, since we're in the base namespace.

Done.

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
base/threading/thread_id_name_manager.h:54: std::map<std::string,
base::InternedString> name_to_interned_name_;
On 2013/01/17 23:18:39, jar wrote:
> nit: remove "base::"

Done.

https://codereview.chromium.org/11438022/diff/87001/base/threading/thread_id_...
base/threading/thread_id_name_manager.h:56: base::InternedString
current_interned_name_;
On 2013/01/17 23:18:39, jar wrote:
> Why not use something like interned_name_to_name_.size(), rather than having
> this redundant counter?
> 
> If you really like the counter, a better name would be:
> next_unused_interned_id_

Done.

[jar (doing other things), 2013-01-24 01:05:14]

https://codereview.chromium.org/11438022/diff/101001/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/11438022/diff/101001/base/threading/thread.cc...
base/threading/thread.cc:69:
ThreadIdNameManager::GetInstance()->RemoveNameForId(thread_id_);
I would have expected that you'd leak this.  Is there anyone that would still
point to the interned (and I'd presumed, highly persistent) string?

https://codereview.chromium.org/11438022/diff/101001/base/threading/thread_id...
File base/threading/thread_id_name_manager.cc (right):

https://codereview.chromium.org/11438022/diff/101001/base/threading/thread_id...
base/threading/thread_id_name_manager.cc:46: name_to_interned_name_[str_name] =
str_name;
I'm a little confused now, as you set values into name_to_interned_name, but you
never read them or use them.

https://codereview.chromium.org/11438022/diff/101001/base/threading/thread_id...
base/threading/thread_id_name_manager.cc:48: id_to_interned_name_[id] =
str_name;
nit: suggest thread_id_to_interned_name

I was confused, as I assumed "id" was the id for the atom we saved away.

https://codereview.chromium.org/11438022/diff/101001/base/threading/thread_id...
base/threading/thread_id_name_manager.cc:51: const char*
ThreadIdNameManager::GetNameForId(PlatformThreadId id) {
nit: Clearer might be GetNameForThread(...), or possibly even GetName(...) since
the caller would prefix it with a ThreadIdNameManager

https://codereview.chromium.org/11438022/diff/101001/base/threading/thread_id...
base/threading/thread_id_name_manager.cc:55: return iter->second.c_str();
This sort of return tends to suggest that the string is very long lived.  Care
must be taken that the caller knows that the  char* will go away :-/.  ...
unless you decide to leak.

https://codereview.chromium.org/11438022/diff/101001/base/threading/thread_id...
File base/threading/thread_id_name_manager.h (right):

https://codereview.chromium.org/11438022/diff/101001/base/threading/thread_id...
base/threading/thread_id_name_manager.h:41: // protectes id_to_name_
nit: "protectEs" --> "Protects"

Also add a period, and make it a sentence.

[dsinclair, 2013-01-24 15:13:05]

https://codereview.chromium.org/11438022/diff/101001/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/11438022/diff/101001/base/threading/thread.cc...
base/threading/thread.cc:69:
ThreadIdNameManager::GetInstance()->RemoveNameForId(thread_id_);
The RemoveNameForId will remove the thread_id -> name mapping. The name still
exists in the name -> interned_name mapping which we'll lookup from next time we
want to use name. That item never gets free'd so it's essentially leaked.


On 2013/01/24 01:05:14, jar wrote:
> I would have expected that you'd leak this.  Is there anyone that would still
> point to the interned (and I'd presumed, highly persistent) string?

https://codereview.chromium.org/11438022/diff/101001/base/threading/thread_id...
File base/threading/thread_id_name_manager.cc (right):

https://codereview.chromium.org/11438022/diff/101001/base/threading/thread_id...
base/threading/thread_id_name_manager.cc:46: name_to_interned_name_[str_name] =
str_name;
Bug, I should have used iter->second in the assignment below instead of
str_name. Fixed now.


On 2013/01/24 01:05:14, jar wrote:
> I'm a little confused now, as you set values into name_to_interned_name, but
you
> never read them or use them.
>

https://codereview.chromium.org/11438022/diff/101001/base/threading/thread_id...
base/threading/thread_id_name_manager.cc:48: id_to_interned_name_[id] =
str_name;
On 2013/01/24 01:05:14, jar wrote:
> nit: suggest thread_id_to_interned_name
> 
> I was confused, as I assumed "id" was the id for the atom we saved away.

Done.

https://codereview.chromium.org/11438022/diff/101001/base/threading/thread_id...
base/threading/thread_id_name_manager.cc:51: const char*
ThreadIdNameManager::GetNameForId(PlatformThreadId id) {
On 2013/01/24 01:05:14, jar wrote:
> nit: Clearer might be GetNameForThread(...), or possibly even GetName(...)
since
> the caller would prefix it with a ThreadIdNameManager

Done.

https://codereview.chromium.org/11438022/diff/101001/base/threading/thread_id...
base/threading/thread_id_name_manager.cc:55: return iter->second.c_str();
This should be long lived as it will continue to live in the name ->
interned_name hash even after the id -> name mapping is removed.


On 2013/01/24 01:05:14, jar wrote:
> This sort of return tends to suggest that the string is very long lived.  Care
> must be taken that the caller knows that the  char* will go away :-/.  ...
> unless you decide to leak.

https://codereview.chromium.org/11438022/diff/101001/base/threading/thread_id...
File base/threading/thread_id_name_manager.h (right):

https://codereview.chromium.org/11438022/diff/101001/base/threading/thread_id...
base/threading/thread_id_name_manager.h:41: // protectes id_to_name_
On 2013/01/24 01:05:14, jar wrote:
> nit: "protectEs" --> "Protects"
> 
> Also add a period, and make it a sentence.

Done.

[jar (doing other things), 2013-01-25 03:13:25]

Two nits... and one optional nit... and then LGTM.

https://codereview.chromium.org/11438022/diff/106002/base/threading/thread_id...
File base/threading/thread_id_name_manager.cc (right):

https://codereview.chromium.org/11438022/diff/106002/base/threading/thread_id...
base/threading/thread_id_name_manager.cc:27: g_default_name = new
std::string(kDefaultName);
nit: probably AutoLock locked(lock_); before writing to the map.

You could trust protection from the singleton... but it is good to consistently
use the lock.

https://codereview.chromium.org/11438022/diff/106002/base/threading/thread_id...
base/threading/thread_id_name_manager.cc:55: thread_id_to_interned_name_[id] =
leaked_str;
nit: I think this code went back and forth.  I thought it might be cleaner to
replace lines 48-55 with:
std::string* leaked_str = NULL;
if (iter != name_to_interned_name_.end()) {
  leaked_string = iter->second;
} else {
  leaked_string = new std::string(str_name);
  thread_id_to_interned_name[id] = leaked_string;
}
thread_id_to_interned_name_[id] = leaked_string;

...but they are both the same length.... so perhaps yours is as good.

I'll let you decide.  I'm ok with either.

https://codereview.chromium.org/11438022/diff/106002/base/threading/thread_id...
File base/threading/thread_id_name_manager.h (right):

https://codereview.chromium.org/11438022/diff/106002/base/threading/thread_id...
base/threading/thread_id_name_manager.h:41: // protects id_to_name_.
nit: Make a full sentence, such as:

lock_ protects both maps id_to_name_ and name_to_interned_name_.

[dsinclair, 2013-01-25 14:54:43]

https://codereview.chromium.org/11438022/diff/106002/base/threading/thread_id...
File base/threading/thread_id_name_manager.cc (right):

https://codereview.chromium.org/11438022/diff/106002/base/threading/thread_id...
base/threading/thread_id_name_manager.cc:27: g_default_name = new
std::string(kDefaultName);
On 2013/01/25 03:13:25, jar wrote:
> nit: probably AutoLock locked(lock_); before writing to the map.
> 
> You could trust protection from the singleton... but it is good to
consistently
> use the lock.

Done.

https://codereview.chromium.org/11438022/diff/106002/base/threading/thread_id...
base/threading/thread_id_name_manager.cc:55: thread_id_to_interned_name_[id] =
leaked_str;
Sorry about that, juggled it around again when I was fixing up the leaking.


On 2013/01/25 03:13:25, jar wrote:
> nit: I think this code went back and forth.  I thought it might be cleaner to
> replace lines 48-55 with:
> std::string* leaked_str = NULL;
> if (iter != name_to_interned_name_.end()) {
>   leaked_string = iter->second;
> } else {
>   leaked_string = new std::string(str_name);
>   thread_id_to_interned_name[id] = leaked_string;
> }
> thread_id_to_interned_name_[id] = leaked_string;
> 
> ...but they are both the same length.... so perhaps yours is as good.
> 
> I'll let you decide.  I'm ok with either.

https://codereview.chromium.org/11438022/diff/106002/base/threading/thread_id...
File base/threading/thread_id_name_manager.h (right):

https://codereview.chromium.org/11438022/diff/106002/base/threading/thread_id...
base/threading/thread_id_name_manager.h:41: // protects id_to_name_.
On 2013/01/25 03:13:25, jar wrote:
> nit: Make a full sentence, such as:
> 
> lock_ protects both maps id_to_name_ and name_to_interned_name_.

Done.

[jar (doing other things), 2013-01-25 17:07:45]

lgtm

[commit-bot: I haz the power, 2013-01-25 17:12:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dsinclair@chromium.org/11438022/110006

[commit-bot: I haz the power, 2013-01-25 20:41:20]

Change committed as 178886