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#newcode446
> 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#newcode435
> 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?r1=49212&r2=49211&pathrev=49212
>
> 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=Function_Names#Function_Names
>
> 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=Function_Names#Function_Names
>
> 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.
http://codereview.chromium.org/6070009/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
