Added labelled thread names to help with some debugging activity. Right now,...

Added labelled thread names to help with some debugging activity.  Right now,
the only platform that it works on is linux (using the prctl API to set the
names of the threads).  Other platforms are setup to build properly if the
flag is set, but their thread names are not currently set.

Committed: http://code.google.com/p/v8/source/detail?r=6141

[marklam, 2010-12-22 20:06:19]

Søren, I'm not sure you're the right person for this review.  If not, can you
please pass this on to the right person?  Thanks.

This change is a small change to add names to threads so that they can be
identified by external tools when debugging v8 activity in a system.  By default
this code is disabled, and should not anyone that does not wish to enable it. 
This change has been tested with a release build with the debugthreadnames
option enabled and disabled.

I only added the option to SConstruct, but it should not break builds using gyp
or other projects.  The feature will just be disabled on those.

[Søren Thygesen Gjesse, 2011-01-03 07:48:34]

http://codereview.chromium.org/6070009/diff/33001/SConstruct
File SConstruct (right):

http://codereview.chromium.org/6070009/diff/33001/SConstruct#newcode129
SConstruct:129: 'CPPDEFINES':   ['DEBUG_THREAD_NAMES'],
Why do you want to enable/disable this using conditional compilation? It does
not seem to add any overhead except for the 16 char name for each thread. If
there are other penalties then I suggest that a flag --threadnames is added to
skip the OS call to actually set the name for the thread.

http://codereview.chromium.org/6070009/diff/33001/src/platform-freebsd.cc
File src/platform-freebsd.cc (right):

http://codereview.chromium.org/6070009/diff/33001/src/platform-freebsd.cc#new...
src/platform-freebsd.cc:446: name_[sizeof(name_)-1] = '\0';
Please add spaces on both sides of '-'. In all the other platform files as well.

http://codereview.chromium.org/6070009/diff/33001/src/platform-macos.cc
File src/platform-macos.cc (right):

http://codereview.chromium.org/6070009/diff/33001/src/platform-macos.cc#newco...
src/platform-macos.cc:435: // one) so we initialize it here too.
Is this not supported on MacOS somehow?

http://codereview.chromium.org/6070009/diff/33001/src/platform.h
File src/platform.h (right):

http://codereview.chromium.org/6070009/diff/33001/src/platform.h#newcode402
src/platform.h:402: inline const char* Name() const {
Please use all lower-case for this type of accessor, see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/6070009/diff/33001/src/platform.h#newcode430
src/platform.h:430: void SetName(const char *name);
This setter should be called set_name, see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/6070009/diff/33001/src/platform.h#newcode436
src/platform.h:436: char name_[16];
I think that this constant deserves a comment. E.g. "The thread name length
limit on Linux is 16.". Actually you should also make a constant for it

static const int kMaxThreadNamelength = 16;

[marklam, 2011-01-04 01:47:09]

FYI, the changes for SConstruct is now removed because we're no longer doing a
conditional flag to set DEBUG_THREAD_NAME.

http://codereview.chromium.org/6070009/diff/33001/SConstruct
File SConstruct (right):

http://codereview.chromium.org/6070009/diff/33001/SConstruct#newcode129
SConstruct:129: 'CPPDEFINES':   ['DEBUG_THREAD_NAMES'],
On 2011/01/03 07:48:34, Søren Gjesse wrote:
> Why do you want to enable/disable this using conditional compilation? It does
> not seem to add any overhead except for the 16 char name for each thread. If
> there are other penalties then I suggest that a flag --threadnames is added to
> skip the OS call to actually set the name for the thread.

I didn't know how strict we want to be about not incurring any cost that may not
yield any benefits for the various platforms.  I agree that the cost here is
minimal, and I'm fine with removing the conditional nature of it.  I'll make the
appropriate change for making this unconditional.  Thanks.

http://codereview.chromium.org/6070009/diff/33001/src/platform-freebsd.cc
File src/platform-freebsd.cc (right):

http://codereview.chromium.org/6070009/diff/33001/src/platform-freebsd.cc#new...
src/platform-freebsd.cc:446: name_[sizeof(name_)-1] = '\0';
On 2011/01/03 07:48:34, Søren Gjesse wrote:
> Please add spaces on both sides of '-'. In all the other platform files as
well.

Done.

http://codereview.chromium.org/6070009/diff/33001/src/platform-macos.cc
File src/platform-macos.cc (right):

http://codereview.chromium.org/6070009/diff/33001/src/platform-macos.cc#newco...
src/platform-macos.cc:435: // one) so we initialize it here too.
On 2011/01/03 07:48:34, Søren Gjesse wrote:
> Is this not supported on MacOS somehow?

There is a pthread_setname_np() but it is not supported before macos 10.6.  I've
adapted an implementation from the chromium sources here:
http://src.chromium.org/viewvc/chrome/trunk/src/base/platform_thread_mac.mm?r...

Is this acceptable?

http://codereview.chromium.org/6070009/diff/33001/src/platform.h
File src/platform.h (right):

http://codereview.chromium.org/6070009/diff/33001/src/platform.h#newcode402
src/platform.h:402: inline const char* Name() const {
On 2011/01/03 07:48:34, Søren Gjesse wrote:
> Please use all lower-case for this type of accessor, see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Done.

http://codereview.chromium.org/6070009/diff/33001/src/platform.h#newcode430
src/platform.h:430: void SetName(const char *name);
On 2011/01/03 07:48:34, Søren Gjesse wrote:
> This setter should be called set_name, see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Done.

http://codereview.chromium.org/6070009/diff/33001/src/platform.h#newcode436
src/platform.h:436: char name_[16];
On 2011/01/03 07:48:34, Søren Gjesse wrote:
> I think that this constant deserves a comment. E.g. "The thread name length
> limit on Linux is 16.". Actually you should also make a constant for it
> 
> static const int kMaxThreadNamelength = 16;

Done.

[Søren Thygesen Gjesse, 2011-01-04 08:43:49]

On 2011/01/04 01:47:09, marklam wrote:
> FYI, the changes for SConstruct is now removed because we're no longer doing a
> conditional flag to set DEBUG_THREAD_NAME.
> 
> http://codereview.chromium.org/6070009/diff/33001/SConstruct
> File SConstruct (right):
> 
> http://codereview.chromium.org/6070009/diff/33001/SConstruct#newcode129
> SConstruct:129: 'CPPDEFINES':   ['DEBUG_THREAD_NAMES'],
> On 2011/01/03 07:48:34, Søren Gjesse wrote:
> > Why do you want to enable/disable this using conditional compilation? It
does
> > not seem to add any overhead except for the 16 char name for each thread. If
> > there are other penalties then I suggest that a flag --threadnames is added
to
> > skip the OS call to actually set the name for the thread.
> 
> I didn't know how strict we want to be about not incurring any cost that may
not
> yield any benefits for the various platforms.  I agree that the cost here is
> minimal, and I'm fine with removing the conditional nature of it.  I'll make
the
> appropriate change for making this unconditional.  Thanks.
> 
> http://codereview.chromium.org/6070009/diff/33001/src/platform-freebsd.cc
> File src/platform-freebsd.cc (right):
> 
>
http://codereview.chromium.org/6070009/diff/33001/src/platform-freebsd.cc#new...
> src/platform-freebsd.cc:446: name_[sizeof(name_)-1] = '\0';
> On 2011/01/03 07:48:34, Søren Gjesse wrote:
> > Please add spaces on both sides of '-'. In all the other platform files as
> well.
> 
> Done.
> 
> http://codereview.chromium.org/6070009/diff/33001/src/platform-macos.cc
> File src/platform-macos.cc (right):
> 
>
http://codereview.chromium.org/6070009/diff/33001/src/platform-macos.cc#newco...
> src/platform-macos.cc:435: // one) so we initialize it here too.
> On 2011/01/03 07:48:34, Søren Gjesse wrote:
> > Is this not supported on MacOS somehow?
> 
> There is a pthread_setname_np() but it is not supported before macos 10.6. 
I've
> adapted an implementation from the chromium sources here:
>
http://src.chromium.org/viewvc/chrome/trunk/src/base/platform_thread_mac.mm?r...
> 
> Is this acceptable?
> 
> http://codereview.chromium.org/6070009/diff/33001/src/platform.h
> File src/platform.h (right):
> 
> http://codereview.chromium.org/6070009/diff/33001/src/platform.h#newcode402
> src/platform.h:402: inline const char* Name() const {
> On 2011/01/03 07:48:34, Søren Gjesse wrote:
> > Please use all lower-case for this type of accessor, see
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> 
> Done.
> 
> http://codereview.chromium.org/6070009/diff/33001/src/platform.h#newcode430
> src/platform.h:430: void SetName(const char *name);
> On 2011/01/03 07:48:34, Søren Gjesse wrote:
> > This setter should be called set_name, see
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> 
> Done.
> 
> http://codereview.chromium.org/6070009/diff/33001/src/platform.h#newcode436
> src/platform.h:436: char name_[16];
> On 2011/01/03 07:48:34, Søren Gjesse wrote:
> > I think that this constant deserves a comment. E.g. "The thread name length
> > limit on Linux is 16.". Actually you should also make a constant for it
> > 
> > static const int kMaxThreadNamelength = 16;
> 
> Done.

LGTM, however I had to add

  #include <sys/prctl.h>

to make it compile on Ubuntu. I also removed the :: in front of prctl.

[Søren Thygesen Gjesse, 2011-01-04 09:15:10]

On 2011/01/04 08:43:49, Søren Gjesse wrote:
> On 2011/01/04 01:47:09, marklam wrote:
> > FYI, the changes for SConstruct is now removed because we're no longer doing
a
> > conditional flag to set DEBUG_THREAD_NAME.
> > 
> > http://codereview.chromium.org/6070009/diff/33001/SConstruct
> > File SConstruct (right):
> > 
> > http://codereview.chromium.org/6070009/diff/33001/SConstruct#newcode129
> > SConstruct:129: 'CPPDEFINES':   ['DEBUG_THREAD_NAMES'],
> > On 2011/01/03 07:48:34, Søren Gjesse wrote:
> > > Why do you want to enable/disable this using conditional compilation? It
> does
> > > not seem to add any overhead except for the 16 char name for each thread.
If
> > > there are other penalties then I suggest that a flag --threadnames is
added
> to
> > > skip the OS call to actually set the name for the thread.
> > 
> > I didn't know how strict we want to be about not incurring any cost that may
> not
> > yield any benefits for the various platforms.  I agree that the cost here is
> > minimal, and I'm fine with removing the conditional nature of it.  I'll make
> the
> > appropriate change for making this unconditional.  Thanks.
> > 
> > http://codereview.chromium.org/6070009/diff/33001/src/platform-freebsd.cc
> > File src/platform-freebsd.cc (right):
> > 
> >
>
http://codereview.chromium.org/6070009/diff/33001/src/platform-freebsd.cc#new...
> > src/platform-freebsd.cc:446: name_[sizeof(name_)-1] = '\0';
> > On 2011/01/03 07:48:34, Søren Gjesse wrote:
> > > Please add spaces on both sides of '-'. In all the other platform files as
> > well.
> > 
> > Done.
> > 
> > http://codereview.chromium.org/6070009/diff/33001/src/platform-macos.cc
> > File src/platform-macos.cc (right):
> > 
> >
>
http://codereview.chromium.org/6070009/diff/33001/src/platform-macos.cc#newco...
> > src/platform-macos.cc:435: // one) so we initialize it here too.
> > On 2011/01/03 07:48:34, Søren Gjesse wrote:
> > > Is this not supported on MacOS somehow?
> > 
> > There is a pthread_setname_np() but it is not supported before macos 10.6. 
> I've
> > adapted an implementation from the chromium sources here:
> >
>
http://src.chromium.org/viewvc/chrome/trunk/src/base/platform_thread_mac.mm?r...
> > 
> > Is this acceptable?
> > 
> > http://codereview.chromium.org/6070009/diff/33001/src/platform.h
> > File src/platform.h (right):
> > 
> > http://codereview.chromium.org/6070009/diff/33001/src/platform.h#newcode402
> > src/platform.h:402: inline const char* Name() const {
> > On 2011/01/03 07:48:34, Søren Gjesse wrote:
> > > Please use all lower-case for this type of accessor, see
> > >
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> > 
> > Done.
> > 
> > http://codereview.chromium.org/6070009/diff/33001/src/platform.h#newcode430
> > src/platform.h:430: void SetName(const char *name);
> > On 2011/01/03 07:48:34, Søren Gjesse wrote:
> > > This setter should be called set_name, see
> > >
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> > 
> > Done.
> > 
> > http://codereview.chromium.org/6070009/diff/33001/src/platform.h#newcode436
> > src/platform.h:436: char name_[16];
> > On 2011/01/03 07:48:34, Søren Gjesse wrote:
> > > I think that this constant deserves a comment. E.g. "The thread name
length
> > > limit on Linux is 16.". Actually you should also make a constant for it
> > > 
> > > static const int kMaxThreadNamelength = 16;
> > 
> > Done.
> 
> LGTM, however I had to add
> 
>   #include <sys/prctl.h>
> 
> to make it compile on Ubuntu. I also removed the :: in front of prctl.

Committed: http://code.google.com/p/v8/source/detail?r=6141

Thank you for the patch.