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.
http://codereview.chromium.org/6070009/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
