720778 - Important threads should have a name for better debugability

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Honza Bambas (:mayhemer)]

I'm never able to find the thread in the debugger I'm just looking for.  We have some 30+ threads running, hard to find the one I need.

nsIThreadInternal (or even nsIThread) should be able to set a name for the thread that would be visible in the debugger then.  This is platform dependent, but at least for windows this would be useful.

The important threads like networking thread and cert verification pool threads should have a name.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Honza, are you working on this?

Comment 2

[Honza Bambas (:mayhemer)]

(In reply to ben turner [:bent] from comment #1)
> Honza, are you working on this?

In one or two days I plan to start.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Honza, I have a patch to do this on Windows that I am cleaning up and expanding. I think it will help you.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Screenshot of VS2010 with most Gecko threads named

Here is a screenshot that shows the effects of my patch in VS2010.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Add PR_CreateLabeledThread to NSPR (Windows only); update Gecko to use it

I added a new function, PR_CreateLabeledThread, to NSPR, which is just like PR_CreatetThread, except that it takes an additional, optional string argument that is used as the threads name.

This patch has several problems:

1. I only made the modifications for Windows. It will fail to compile on other platforms until the changes to NSPR for non-Windows platforms are made. I did not look into what is required to label threads in gdb.

2. There are several places where I number threads in thread pools (e.g. "SSL Cert Verification #0," "SSL Cert Verification #1."). Somewhat ironically, the numbering logic may not be thread-safe in all cases.

3. When writing this patch, I found there are a few different ways that we are doing thread pooling. This required duplicating the (small) thread numbering logic in multiple places.

4. It seems I have missed some threads; I am not quite sure where though.

5. AFAICT, the mechanism I use to name the threads only works when a debugger is attached when the thread is created. (I don't know if BreakPad counts as a debugger or not.) But, effectively, this means that you are much better off with this patch when you start Firefox from within the debugger, AFAICT.

6. The patch needs to be split into a NSPR portion and one or more Gecko portions.

7. Some of the thread names I chose might not be the best.

8. I do not know if it is reasonable to change the nsIThread* interfaces in this way, or if we instead need to create new nsIThread*2 interfaces for the new methods. (Also, I suggest that we mark these nsIThread* interfaces as nativeonly in the IDL.)

9. The Gecko wrappers around PR_CreateLabeledThread need to have the label as the first parameter, because they use default values for their last parameters. But, my PR_CreateLabeledThread takes the label as its last parameter. Perhaps we should change PR_CreateLabeledThread to take the label as the first parameter.

The actual mechanism is copy/pasted/modified from the MSDN documentation:
http://msdn.microsoft.com/en-us/library/xcb2z8hs.aspx

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

10. I did not measure whether adding the thread name causes any performance problem--especially in the case where the debugger isn't attached. It might be the case that we need to avoid naming the thread unless IsDebuggerPresent() returns true.

11. The change to NSPR will cause a memory leak because the copy of the string that contains the thread name is never freed.

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 597361 [details] [diff] [review]
Add PR_CreateLabeledThread to NSPR (Windows only); update Gecko to use it

Review of attachment 597361 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, that is mostly what I want to do my self.

For me this in general looks good.

I think we could also name threads after attach (I personally do that often).  In a debug build we may have a dedicated watch dog thread that polls for IsDebuggerPresent() result and invokes naming on all running threads when changed to TRUE (we have to save names in some id+name array ; please also see my other proposal bellow).

For purposes of logging I also think of making the name programmatically available to the application it self (i.e. having const char * PR_GetCurrentThreadName())


At least for posix you can 
  #include <sys/prctl.h>
  prctl(PR_SET_NAME,"<null> terminated string",0,0,0)

The limit for the name is 15 bytes + trailing \0.

::: nsprpub/pr/src/md/windows/w95thred.c
@@ +139,5 @@
> +   __try
> +   {
> +      RaiseException( MS_VC_EXCEPTION, 0, sizeof(info)/sizeof(ULONG_PTR), (ULONG_PTR*)&info );
> +   }
> +   __except(EXCEPTION_EXECUTE_HANDLER)

According the comment in MS docs, EXCEPTION_CONTINUE_EXECUTION might be needed.

@@ +149,5 @@
>  pr_root(void *arg)
>  {
>      PRThread *thread = (PRThread *)arg;
> +    if (thread->optionalLabel) {
> +        SetThreadName(thread->optionalLabel);

Free thread->optionalLabel

::: xpcom/threads/nsThreadManager.cpp
@@ +255,5 @@
>      return NS_ERROR_OUT_OF_MEMORY;
>    NS_ADDREF(thr);
>  
> +  const char * label = optionalLabel.Length() > 0
> +                     ? PromiseFlatCString(optionalLabel).get()

Hmm.. isn't the result of .get() immediately freed?

Comment 8

[Josh Matthews [:jdm]]

http://stackoverflow.com/questions/2369738/can-i-set-the-name-of-a-thread-in-pthreads-linux
http://stackoverflow.com/questions/2057960/how-to-set-a-threadname-in-macosx

Looks like there's a pthreads API for this which works for GDB, and a Cocoa API which would work for things like Shark.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I'm not sure if we should be messing with NSPR here... Can't we just fix nsIThread and then make Gecko use that API wherever possible?

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to ben turner [:bent] from comment #9)
> I'm not sure if we should be messing with NSPR here... Can't we just fix
> nsIThread and then make Gecko use that API wherever possible?

IMO, We should rewrite nsIThread* and NS_New*Thread to use either POSIX or Windows threading directly, instead of NSPR, put the naming logic in NS_NewLabeledThread, replace all uses of PR_CreateThread in Gecko with NS_NewLabeledThread, and drop NS_NewThread and then (eventually) drop NSPR threading from the Gecko API. AFAICT, this project would not as much work as it sounds, but I am not sure replacing NSPR threading in Gecko should be a high priority.

In the interim, if we don't need to support naming on attaching a debugger, we could do the naming logic in NS_NewLabeledThread. That would work well because we can pass -1 as the thread ID when the thread we are labeling is the current thread. 

However, if we want to support naming on attach, then we would need to know the OS-level thread ID, and that might mean doing the work in NSPR, since NSPR hides that from us (I think).

(In reply to Honza Bambas (:mayhemer) from comment #7)
> I think we could also name threads after attach (I personally do that
> often).  In a debug build we may have a dedicated watch dog thread that
> polls for IsDebuggerPresent() result and invokes naming on all running
> threads when changed to TRUE (we have to save names in some id+name array ;
> please also see my other proposal bellow).

We should be able to do this IsDebuggerPresent() check in the main event loop. We will also need to do it in NS_NewLabeledThread/PR_CreateLabeledThread.

We should try to do this in production builds too--and we should try to get BreakPad to spit out the thread names in its stack traces. (I wonder if thread names are visible in minidumps.)

> For purposes of logging I also think of making the name programmatically
> available to the application it self (i.e. having const char *
> PR_GetCurrentThreadName())

Sure, it might be useful logging.

> At least for posix you can 
>   #include <sys/prctl.h>
>   prctl(PR_SET_NAME,"<null> terminated string",0,0,0)
> 
> The limit for the name is 15 bytes + trailing \0.

Does this also work only when a debugger is attached?

Definitely my patch isn't production-quality code; I just wanted to share with you what I've been using to debug stuff myself.

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Brian Smith (:bsmith) from comment #10)
> However, if we want to support naming on attach, then we would need to know
> the OS-level thread ID, and that might mean doing the work in NSPR, since
> NSPR hides that from us (I think).

If we replace PR_CreateThread with direct call to OS API then not.

> We should be able to do this IsDebuggerPresent() check in the main event
> loop. We will also need to do it in
> NS_NewLabeledThread/PR_CreateLabeledThread.

It won't work in case you attach after the main thread deadlock, what is bad and exactly in that case would be useful.  But we must consider the weight here.

> Definitely my patch isn't production-quality code; I just wanted to share
> with you what I've been using to debug stuff myself.

Sure.

Comment 12

[Honza Bambas (:mayhemer)]

Brian, don't forget please to convert modified files to UNIX line ending.

Comment 13

[Honza Bambas (:mayhemer)]

I'd love to have a following function:

+PR_IMPLEMENT(const char *) PR_GetThreadLabel(const PRThread *thread)
+{
+    return thread->optionalLabel;
+}


This means to manage the names on the NSPR level.  Can be used for NSPR logging, or for the profiler I work on (bug 729182), if found valid and useful.

Comment 14

[Honza Bambas (:mayhemer)]

Attachment: PR_GetThreadLabel API

As demanded in comment 13.  Apply on top of the Brian's patch.

Comment 15

[Honza Bambas (:mayhemer)]

Brian, the patch could not build in opt config.  You are missing #include "nsString.h" in xpcom/threads/nsThreadPool.h.

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Honza, I saw on the team priorities page that you are unsure if I am taking this. I would not have time to finish this until after April. If there is some part of my patch that is usable, and you'd like me to clean it up sometime soon so that you can r+ it and then build your enhancements on top of it, let me know.

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to Brian Smith (:bsmith) from comment #16)
> Honza, I saw on the team priorities page that you are unsure if I am taking
> this. I would not have time to finish this until after April. If there is
> some part of my patch that is usable, and you'd like me to clean it up
> sometime soon so that you can r+ it and then build your enhancements on top
> of it, let me know.

Brian, first I have to say I love your patch!  Since this blocks my work on the visual profiler that seems to be useful even in early stages, then I can take this bug, add those few nits I've found to make it work and do reviews.  I really want this to be on the NSPR level to make it available for NSPR logging and for the profiler.  However, the profiler at least could be altered to use naming on the XPCOM level however it may be a perf regression/code complication if wtc does strongly disagree with changing this on the NSPR level.

Comment 18

[Honza Bambas (:mayhemer)]

Attachment: v2, for reference to make patches from bug 729182 build

Comment 19

[Honza Bambas (:mayhemer)]

For the record:

/builds/slave/try-lnx/build/obj-firefox/js/src/shell/../../../../js/src/shell/js.cpp:3456: undefined reference to `PR_CreateLabeledThread'
jsworkers.o: In function `js::workers::ThreadPool::start(JSContext*)':
/builds/slave/try-lnx/build/obj-firefox/js/src/shell/../../../../js/src/shell/jsworkers.cpp:535: undefined reference to `PR_CreateLabeledThread'
/builds/slave/try-lnx/build/obj-firefox/js/src/shell/../../../../js/src/shell/jsworkers.cpp:535: undefined reference to `PR_CreateLabeledThread'
/builds/slave/try-lnx/build/obj-firefox/js/src/shell/../../../../js/src/shell/jsworkers.cpp:535: undefined reference to `PR_CreateLabeledThread'
/builds/slave/try-lnx/build/obj-firefox/js/src/shell/../../../../js/src/shell/jsworkers.cpp:535: undefined reference to `PR_CreateLabeledThread'
jsworkers.o:/builds/slave/try-lnx/build/obj-firefox/js/src/shell/../../../../js/src/shell/jsworkers.cpp:535: more undefined references to `PR_CreateLabeledThread' follow
collect2: ld returned 1 exit status
make[6]: Leaving directory `/builds/slave/try-lnx/build/obj-firefox/js/src/shell'
make[6]: *** [js] Error 1
make[5]: *** [libs] Error 2

This happens on almost all platforms.

Comment 20

[Honza Bambas (:mayhemer)]

My feedback:
- definitely needs an API to get the label of a thread programmatically (blocks bug 729182)
- the primary goal is to make this (^) API work, regardless a platform and build config (the current patch builds on win-debug only)
- the secondary goal, finished for non-windows in followups (by me?), is to provide the name to debuggers
- I don't have a strong opinion where to set and store the name (NSPR or XPCOM) ; NSPR is more straightforward IMO, but when we don't want to modify NSPR APIs then having it on the XPCOM level is probably OK too, however less performance efficient and more cumbersome ; it means to strictly use NS_NewThread and not PR_CreateThread (is that doable?)


Brian, as we talked about it on IRC, the plan is:
- you update and r? the patch
- I'll start to work on the support for adding names available to debuggers on other platforms

Comment 21

[Honza Bambas (:mayhemer)]

And a bit more feedback:
- I think LazyIdleThread should have a customizable name too

Comment 22

[Wan-Teh Chang]

Honza: looking at the PlatformThread::SetName() method implementations
in the Chromium tree, I believe we can add a PR_SetThreadName() function,
and still use PR_CreateThread to create a thread.

Chromium has implementations for Windows (same technique you're using),
Mac OS X (10.6 or later), and Linux (except for the main thread).

See
http://stackoverflow.com/questions/2057960/how-to-set-a-threadname-in-macosx
http://stackoverflow.com/questions/2369738/can-i-set-the-name-of-a-thread-in-pthreads-linux

Comment 23

[Benoit Girard (:BenWa)]

I wrote a patch in bug 734685 to cover PlatformThread::SetName() in Mac you should use for this bug.

Comment 24

[Honza Bambas (:mayhemer)]

(In reply to Wan-Teh Chang from comment #22)
> Honza: looking at the PlatformThread::SetName() method implementations
> in the Chromium tree, I believe we can add a PR_SetThreadName() function,
> and still use PR_CreateThread to create a thread.
> 
> Chromium has implementations for Windows (same technique you're using),
> Mac OS X (10.6 or later), and Linux (except for the main thread).
> 
> See
> http://stackoverflow.com/questions/2057960/how-to-set-a-threadname-in-macosx
> http://stackoverflow.com/questions/2369738/can-i-set-the-name-of-a-thread-in-
> pthreads-linux

To recapitulate before I start updating the patch:
- let's don't change or extend the PR_CreateThread API
- let's have a new PR_SetThreadLabel and PR_GetThreadLabel functions
- PR_SetThreadLabel will save the label in PRThread struct and call new _PR_MD_SET_THREAD_LABEL that will be platform specific
- PR_GetThreadLabel will read the label from PRThread struct

I want to confirm this is the API that you accept and that the idea of new _PR_MD -> _MD function is the right one for this case.


On the other hand, there is also bug 606102, that wants to add this with newNamedThread method on the XPCOM level.  According we would like to get rid of NSPR ones, it may be smarter to go that way instead of extending NSPR.

Chris?

Comment 25

[Honza Bambas (:mayhemer)]

Attachment: Part1 v1

This is WIP (mostly done).  I just have following issue when building on our linux try:

/tools/python/bin/python2.5 /builds/slave/try-lnx/build/js/src/config/pythonpath.py -I../config /builds/slave/try-lnx/build/js/src/config/expandlibs_exec.py --uselist --  /usr/bin/ccache /tools/gcc-4.5-0moz3/bin/g++ -o js  -fno-rtti -pedantic -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wcast-align -Wno-long-long -ffunction-sections -fdata-sections -pthread -pipe  -DNDEBUG -DTRIMMED -g -O3 -freorder-blocks -finline-limit=50 -fomit-frame-pointer js.o jsworkers.o jsoptparse.o jsheaptools.o   -lpthread    -Wl,-rpath-link,../../../dist/bin -Wl,-rpath-link,/builds/slave/try-lnx/build/obj-firefox/dist/lib   -L../../../dist/bin -L../../../dist/lib -L/builds/slave/try-lnx/build/obj-firefox/dist/lib -lplds4 -lplc4 -lnspr4 -lpthread -ldl ../editline/libeditline.a ../libjs_static.a -Wl,--whole-archive ../../../dist/lib/libmozglue.a -Wl,--no-whole-archive -rdynamic -ldl    
> ../../../dist/bin/libnspr4.so: undefined reference to `pthread_setname_np'
collect2: ld returned 1 exit status
make[6]: *** [js] Error 1
make[6]: Leaving directory `/builds/slave/try-lnx/build/obj-firefox/js/src/shell'
make[5]: *** [libs] Error 2
make[5]: Leaving directory `/builds/slave/try-lnx/build/obj-firefox/js/src'
make[4]: *** [libs_tier_js] Error 2
make[4]: Leaving directory `/builds/slave/try-lnx/build/obj-firefox'
make[3]: *** [tier_js] Error 2
make[3]: Leaving directory `/builds/slave/try-lnx/build/obj-firefox'
make[2]: *** [default] Error 2
make[2]: Leaving directory `/builds/slave/try-lnx/build/obj-firefox'
make[1]: *** [realbuild] Error 2
make[1]: Leaving directory `/builds/slave/try-lnx/build'
make: *** [build] Error 2

Cannot reproduce on my Fedora installation.

If anyone has any advice, please provide it.

Comment 26

[Honza Bambas (:mayhemer)]

Attachment: Part2 v1

This adds usage of the new API where appropriate.

Comment 27

[Honza Bambas (:mayhemer)]

Comment on attachment 614558 [details] [diff] [review]
Part1 v1

Review of attachment 614558 [details] [diff] [review]:
-----------------------------------------------------------------

::: nsprpub/pr/src/pthreads/ptthread.c
@@ +1664,5 @@
> +    int (*dynamic_pthread_setname_np)(const char*);
> +    *(void**)(&dynamic_pthread_setname_np) =
> +        dlsym(RTLD_DEFAULT, "pthread_setname_np");
> +    if (!dynamic_pthread_setname_np)
> +        return PR_FAILURE;

I just checked this code doesn't work on OSX 10.7 debug build (dynamic_pthread_setname_np is null).  Changing RTLD_DEFAULT to 0 didn't help.

It is a direct copy of the code from tools/profiler/platform-macos.cc.

Comment 28

[Benoit Girard (:BenWa)]

I get named threads for profiling and as well for the ipc threads on OSX on Lion (non debug):
  19 "SamplerThread"                 0x00007fff83f9d67a in mach_msg_trap ()

This should give you another example:
http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/platform_thread_mac.mm#58

But they are the same so I'm not sure what you're seeing that's different.

Comment 29

[Honza Bambas (:mayhemer)]

(In reply to Benoit Girard (:BenWa) from comment #28)
> But they are the same so I'm not sure what you're seeing that's different.

Thanks.  I just now discovered that RTLD_DEFAULT is -2 on darwin.  Including <dlfcn.h> header helped (that's what I was missing).

Comment 30

[Honza Bambas (:mayhemer)]

And the limit for the name is just 16, not 32.

Comment 31

[Honza Bambas (:mayhemer)]

Attachment: Part1 v2

Thanks Wan-Teh for all your quick and very useful replies in private emails.  You're great!

- implementation for all platforms
- linux and darwin loads dynamically, name len limit 15 bytes
- based on mozilla/nsprpub (needs to introduce a real NSPR patch yet)

Comment 32

[Honza Bambas (:mayhemer)]

Attachment: Part2 v2

- result is similar to what Brian's original patch does
- has some utility class for pool naming

Comment 33

[Honza Bambas (:mayhemer)]

Try revealed a small issue on OSX.  I will update both patches in a short after a new try passes.

Comment 34

[Honza Bambas (:mayhemer)]

Attachment: Part1 v2.1

- added null check for name argument at PR_SetThreadName (crashing on OSX)

Comment 35

[Honza Bambas (:mayhemer)]

Attachment: Part2 v2.1

- fixed LazyThread name argument

Comment 36

[Honza Bambas (:mayhemer)]

Wan-Teh, Brian, any chance on reviewing this in some reasonable time frame?

Comment 37

[Wan-Teh Chang]

Comment on attachment 616097 [details] [diff] [review]
Part1 v2.1

Review of attachment 616097 [details] [diff] [review]:
-----------------------------------------------------------------

Honza: thank you for the patch.  I reviewed the NSPR files in this
patch.  I made some changes and will attach a new NSPR patch.

I describe the more interesting changes I made below.

::: nsprpub/pr/include/private/primpl.h
@@ +1653,5 @@
>      PRBool io_suspended;
>  
>      _MDThread md;
>  #endif
> +    const char * name;

I changed this to "char *name", which is a copy of the thread name
passed to PR_SetThreadName.

::: nsprpub/pr/include/prthread.h
@@ +283,5 @@
>  
> +/*
> +** Set thread label, this will be visible in a debugger and accessible
> +** via call to PR_GetThreadName().  The lable string is not copied
> +** and must be valid for the lifetime of the thread.

I think the requirement that the name string must stay valid
for the lifetime of the thread seems a little restrictive.
Where does this requirement come from?  Windows?

I changed your patch to copy the 'name' argument into the
PRThread structure.  There is a tiny time interval between
the time we destroy the PRThread structure and the time the
thread really dies, in which the debugger (on Windows) could
be pointing to a freed string.  Is that a problem?

::: nsprpub/pr/src/pthreads/ptthread.c
@@ +1658,5 @@
> +        return PR_FAILURE;
> +
> +    thread->name = name;
> +
> +#if defined(XP_UNIX)

I removed this because this file is only compiled on Unix.

I changed the C++ // comments to C /* */ comments.

@@ +1676,5 @@
> +
> +    *(void**)(&dynamic_pthread_setname_np) =
> +        dlsym(RTLD_DEFAULT, "pthread_setname_np");
> +    if (!dynamic_pthread_setname_np)
> +        return PR_FAILURE;

I changed to return PR_SUCCESS here because we should not fail
if pthread_setname_np doesn't exist.

::: nsprpub/pr/src/threads/combined/pruthr.c
@@ +1895,5 @@
> +
> +    thread->name = name;
> +
> +    if (IsDebuggerPresent())
> +        _PR_MD_SET_CURRENT_THREAD_NAME(name);

I moved the IsDebuggerPresent() call into the
_PR_MD_SET_CURRENT_THREAD_NAME function in w95thred.c and
ntthread.c.

Comment 38

[Wan-Teh Chang]

Attachment: NSPR patch, v3 (by Honza Bambas)

Honza: please review and test this NSPR patch.

The key question is whether it is safe to pass a copy of
the thread name in the PRThread structure to
_PR_MD_SET_THREAD_NAME on Windows.  According to this
MSDN page, it is safe:
http://msdn.microsoft.com/en-us/library/xcb2z8hs.aspx

I assume the pthread_set_name_np function will copy the
thread name internally.

Comment 39

[Honza Bambas (:mayhemer)]

Comment on attachment 624276 [details] [diff] [review]
NSPR patch, v3 (by Honza Bambas)

Review of attachment 624276 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab

Thank you.  Tested and works well for me.

Brian: I'll also update the "naming" patch since some of the code has to be moved/updated to/in that patch.  No major changes, though.

Comment 40

[Honza Bambas (:mayhemer)]

Comment on attachment 616097 [details] [diff] [review]
Part1 v2.1

># HG changeset patch
># Parent 667ad7fdd2d18aafbf868e5292eb347013acc26f
>* * *
>try: -b do -p all -u none -t none
>
>diff --git a/nsprpub/pr/include/md/_win95.h b/nsprpub/pr/include/md/_win95.h
>--- a/nsprpub/pr/include/md/_win95.h
>+++ b/nsprpub/pr/include/md/_win95.h
>@@ -389,16 +389,18 @@ extern PROsfd _MD_Accept(PRFileDesc *fd,
> 
> /* --- Threading Stuff --- */
> #define _MD_DEFAULT_STACK_SIZE            0
> #define _MD_INIT_THREAD             _PR_MD_INIT_THREAD
> #define _MD_INIT_ATTACHED_THREAD    _PR_MD_INIT_THREAD
> #define _MD_CREATE_THREAD           _PR_MD_CREATE_THREAD
> #define _MD_YIELD                   _PR_MD_YIELD
> #define _MD_SET_PRIORITY            _PR_MD_SET_PRIORITY
>+#define _MD_SET_CURRENT_THREAD_NAME \
>+                                    _PR_MD_SET_CURRENT_THREAD_NAME
> #define _MD_CLEAN_THREAD            _PR_MD_CLEAN_THREAD
> #define _MD_SETTHREADAFFINITYMASK   _PR_MD_SETTHREADAFFINITYMASK
> #define _MD_GETTHREADAFFINITYMASK   _PR_MD_GETTHREADAFFINITYMASK
> #define _MD_EXIT_THREAD             _PR_MD_EXIT_THREAD
> #define _MD_EXIT                    _PR_MD_EXIT
> #define _MD_SUSPEND_THREAD          _PR_MD_SUSPEND_THREAD
> #define _MD_RESUME_THREAD           _PR_MD_RESUME_THREAD
> #define _MD_SUSPEND_CPU             _PR_MD_SUSPEND_CPU
>diff --git a/nsprpub/pr/include/md/_winnt.h b/nsprpub/pr/include/md/_winnt.h
>--- a/nsprpub/pr/include/md/_winnt.h
>+++ b/nsprpub/pr/include/md/_winnt.h
>@@ -403,16 +403,18 @@ extern int _PR_NTFiberSafeSelect(int, fd
> #define _MD_DEFAULT_STACK_SIZE            0
> #define _MD_INIT_THREAD             _PR_MD_INIT_THREAD
> #define _MD_INIT_ATTACHED_THREAD    _PR_MD_INIT_THREAD
> #define _MD_CREATE_THREAD           _PR_MD_CREATE_THREAD
> #define _MD_JOIN_THREAD             _PR_MD_JOIN_THREAD
> #define _MD_END_THREAD              _PR_MD_END_THREAD
> #define _MD_YIELD                   _PR_MD_YIELD
> #define _MD_SET_PRIORITY            _PR_MD_SET_PRIORITY
>+#define _MD_SET_CURRENT_THREAD_NAME \
>+                                    _PR_MD_SET_CURRENT_THREAD_NAME
> #define _MD_CLEAN_THREAD            _PR_MD_CLEAN_THREAD
> #define _MD_SETTHREADAFFINITYMASK   _PR_MD_SETTHREADAFFINITYMASK
> #define _MD_GETTHREADAFFINITYMASK   _PR_MD_GETTHREADAFFINITYMASK
> #define _MD_EXIT_THREAD             _PR_MD_EXIT_THREAD
> #define _MD_SUSPEND_THREAD          _PR_MD_SUSPEND_THREAD
> #define _MD_RESUME_THREAD           _PR_MD_RESUME_THREAD
> #define _MD_SUSPEND_CPU             _PR_MD_SUSPEND_CPU
> #define _MD_RESUME_CPU              _PR_MD_RESUME_CPU
>diff --git a/nsprpub/pr/include/private/primpl.h b/nsprpub/pr/include/private/primpl.h
>--- a/nsprpub/pr/include/private/primpl.h
>+++ b/nsprpub/pr/include/private/primpl.h
>@@ -1649,16 +1649,17 @@ struct PRThread {
>      * io_suspended flag will be set to true.  The thread will be resumed
>      * but may run into trouble issuing additional IOs until the io_pending
>      * flag can be cleared 
>      */
>     PRBool io_suspended;
> 
>     _MDThread md;
> #endif
>+    const char * name;
> };
> 
> struct PRProcessAttr {
>     PRFileDesc *stdinFd;
>     PRFileDesc *stdoutFd;
>     PRFileDesc *stderrFd;
>     char *currentDirectory;
>     char *fdInheritBuffer;
>diff --git a/nsprpub/pr/include/prthread.h b/nsprpub/pr/include/prthread.h
>--- a/nsprpub/pr/include/prthread.h
>+++ b/nsprpub/pr/include/prthread.h
>@@ -276,11 +276,23 @@ NSPR_API(PRThreadScope) PR_GetThreadScop
> */
> NSPR_API(PRThreadType) PR_GetThreadType(const PRThread *thread);
> 
> /*
> ** Get the join state of this thread.
> */
> NSPR_API(PRThreadState) PR_GetThreadState(const PRThread *thread);
> 
>+/*
>+** Set thread label, this will be visible in a debugger and accessible
>+** via call to PR_GetThreadName().  The lable string is not copied
>+** and must be valid for the lifetime of the thread.
>+*/
>+NSPR_API(PRStatus) PR_SetThreadName(const char *name);
>+
>+/*
>+** Return current thread label, if set.  Otherwise NULL.
>+*/
>+NSPR_API(const char*) PR_GetThreadName();
>+
> PR_END_EXTERN_C
> 
> #endif /* prthread_h___ */
>diff --git a/nsprpub/pr/src/md/windows/ntthread.c b/nsprpub/pr/src/md/windows/ntthread.c
>--- a/nsprpub/pr/src/md/windows/ntthread.c
>+++ b/nsprpub/pr/src/md/windows/ntthread.c
>@@ -304,16 +304,22 @@ void
>     if (!rv) {
> 	PR_LOG(_pr_thread_lm, PR_LOG_MIN,
>                 ("PR_SetThreadPriority: can't set thread priority\n"));
>     }
>     return;
> }
> 
> void
>+_PR_MD_SET_CURRENT_THREAD_NAME(const char *name)
>+{
>+    // TODO Implement for Win NT
>+}
>+
>+void
> _PR_MD_CLEAN_THREAD(PRThread *thread)
> {
>     BOOL rv;
> 
>     if (thread->md.acceptex_buf) {
>         PR_DELETE(thread->md.acceptex_buf);
>     }
> 
>diff --git a/nsprpub/pr/src/md/windows/w95thred.c b/nsprpub/pr/src/md/windows/w95thred.c
>--- a/nsprpub/pr/src/md/windows/w95thred.c
>+++ b/nsprpub/pr/src/md/windows/w95thred.c
>@@ -195,16 +195,53 @@ void
>     PR_ASSERT(rv);
>     if (!rv) {
> 	PR_LOG(_pr_thread_lm, PR_LOG_MIN,
>                 ("PR_SetThreadPriority: can't set thread priority\n"));
>     }
>     return;
> }
> 
>+const DWORD MS_VC_EXCEPTION = 0x406D1388;
>+
>+#pragma pack(push,8)
>+typedef struct tagTHREADNAME_INFO
>+{
>+   DWORD dwType; // Must be 0x1000.
>+   LPCSTR szName; // Pointer to name (in user addr space).
>+   DWORD dwThreadID; // Thread ID (-1=caller thread).
>+   DWORD dwFlags; // Reserved for future use, must be zero.
>+} THREADNAME_INFO;
>+#pragma pack(pop)
>+
>+void
>+_PR_MD_SET_CURRENT_THREAD_NAME(const char *name)
>+{
>+   THREADNAME_INFO info;
>+
>+   if (!name)
>+      return;
>+
>+   info.dwType = 0x1000;
>+   info.szName = (char*) name;
>+   info.dwThreadID = -1;
>+   info.dwFlags = 0;
>+
>+   __try
>+   {
>+      RaiseException(MS_VC_EXCEPTION,
>+                     0,
>+                     sizeof(info) / sizeof(ULONG_PTR),
>+                     (ULONG_PTR*)&info);
>+   }
>+   __except(EXCEPTION_CONTINUE_EXECUTION)
>+   {
>+   }
>+}
>+
> void
> _PR_MD_CLEAN_THREAD(PRThread *thread)
> {
>     BOOL rv;
> 
>     if (thread->md.blocked_sema) {
>         rv = CloseHandle(thread->md.blocked_sema);
>         PR_ASSERT(rv);
>diff --git a/nsprpub/pr/src/nspr.def b/nsprpub/pr/src/nspr.def
>--- a/nsprpub/pr/src/nspr.def
>+++ b/nsprpub/pr/src/nspr.def
>@@ -348,16 +348,18 @@ EXPORTS ;-
> 		PR_SetStdioRedirect;
> 		PR_SetSysfdTableSize;
> 		PR_SetThreadAffinityMask;
> 		PR_SetThreadDumpProc;
> 		PR_SetThreadGCAble;
> 		PR_SetThreadPriority;
> 		PR_SetThreadPrivate;
> 		PR_SetThreadRecycleMode;
>+		PR_SetThreadName;
>+		PR_GetThreadName;
> 		PR_Shutdown;
> 		PR_ShutdownThreadPool;
> 		PR_Sleep;
> 		PR_Socket;
> 		PR_StackPop;
> 		PR_StackPush;
> 		PR_Stat;
> 		PR_StringToNetAddr;
>diff --git a/nsprpub/pr/src/pthreads/ptthread.c b/nsprpub/pr/src/pthreads/ptthread.c
>--- a/nsprpub/pr/src/pthreads/ptthread.c
>+++ b/nsprpub/pr/src/pthreads/ptthread.c
>@@ -46,16 +46,17 @@
> #include "prlog.h"
> #include "primpl.h"
> #include "prpdce.h"
> 
> #include <pthread.h>
> #include <unistd.h>
> #include <string.h>
> #include <signal.h>
>+#include <dlfcn.h>
> 
> #ifdef SYMBIAN
> /* In Open C sched_get_priority_min/max do not work properly, so we undefine
>  * _POSIX_THREAD_PRIORITY_SCHEDULING here.
>  */
> #undef _POSIX_THREAD_PRIORITY_SCHEDULING
> #endif
> 
>@@ -1639,11 +1640,80 @@ PR_IMPLEMENT(void*)PR_GetSP(PRThread *th
> 	thread_tcb = (char*)tid.field1;
> 	top_sp = *(char**)(thread_tcb + 128);
> 	PR_LOG(_pr_gc_lm, PR_LOG_ALWAYS, ("End PR_GetSP %p \n", top_sp));
> 	return top_sp;
> }  /* PR_GetSP */
> 
> #endif /* !defined(_PR_DCETHREADS) */
> 
>+PR_IMPLEMENT(PRStatus) PR_SetThreadName(const char *name)
>+{
>+    PRThread * thread;
>+    int result;
>+
>+    if (!name)
>+        return PR_FAILURE;
>+
>+    thread = PR_GetCurrentThread();
>+    if (!thread)
>+        return PR_FAILURE;
>+
>+    thread->name = name;
>+
>+#if defined(XP_UNIX)
>+
>+#if defined(OPENBSD) || defined(FREEBSD)
>+    result = pthread_set_name_np(thread->id, name);
>+
>+#else // not BSD
>+    // On OSX, pthread_setname_np is only available in 10.6 or later, so test
>+    // for it at runtime.  It also may not be available on all linux distros.
>+    // The name length limit is 16 bytes.
>+#if defined(DARWIN)
>+    int (*dynamic_pthread_setname_np)(const char*);
>+#else
>+    int (*dynamic_pthread_setname_np)(pthread_t, const char*);
>+#endif
>+
>+    *(void**)(&dynamic_pthread_setname_np) =
>+        dlsym(RTLD_DEFAULT, "pthread_setname_np");
>+    if (!dynamic_pthread_setname_np)
>+        return PR_FAILURE;
>+
>+#define SETNAME_LENGTH_CONSTRAINT 15
>+    char name_dup[SETNAME_LENGTH_CONSTRAINT + 1];
>+    if (strlen(name) > SETNAME_LENGTH_CONSTRAINT) {
>+        memcpy(name_dup, name, SETNAME_LENGTH_CONSTRAINT);
>+        name_dup[SETNAME_LENGTH_CONSTRAINT] = 0;
>+        name = name_dup;
>+    }
>+
>+#if defined(DARWIN)
>+    result = dynamic_pthread_setname_np(name);
>+#else
>+    result = dynamic_pthread_setname_np(thread->id, name);
>+#endif
>+
>+#endif // not BSD
>+    if (result)
>+        return PR_FAILURE;
>+
>+    return PR_SUCCESS;
>+
>+#endif // XP_UNIX
>+
>+    return PR_FAILURE;
>+}
>+
>+PR_IMPLEMENT(const char *) PR_GetThreadName()
>+{
>+    PRThread * thread;
>+    thread = PR_GetCurrentThread();
>+    if (!thread)
>+        return NULL;
>+
>+    return thread->name;
>+}
>+
> #endif  /* defined(_PR_PTHREADS) || defined(_PR_DCETHREADS) */
> 
> /* ptthread.c */
>diff --git a/nsprpub/pr/src/threads/combined/pruthr.c b/nsprpub/pr/src/threads/combined/pruthr.c
>--- a/nsprpub/pr/src/threads/combined/pruthr.c
>+++ b/nsprpub/pr/src/threads/combined/pruthr.c
>@@ -1880,8 +1880,33 @@ void
>     _PR_RUNQ_UNLOCK(cpu);
>     if (!_PR_IS_NATIVE_THREAD(me) && (cpu == me->cpu)) {
>         if (pri > me->priority) {
>             _PR_SET_RESCHED_FLAG();
>         }
>     }
> #endif
> }
>+
>+PR_IMPLEMENT(PRStatus) PR_SetThreadName(const char *name)
>+{
>+    PRThread * thread;
>+    thread = PR_GetCurrentThread();
>+    if (!thread)
>+        return PR_FAILURE;
>+
>+    thread->name = name;
>+
>+    if (IsDebuggerPresent())
>+        _PR_MD_SET_CURRENT_THREAD_NAME(name);
>+
>+    return PR_SUCCESS;
>+}
>+
>+PR_IMPLEMENT(const char *) PR_GetThreadName()
>+{
>+    PRThread * thread;
>+    thread = PR_GetCurrentThread();
>+    if (!thread)
>+        return NULL;
>+
>+    return thread->name;
>+}
>diff --git a/xpcom/glue/nsThreadUtils.cpp b/xpcom/glue/nsThreadUtils.cpp
>--- a/xpcom/glue/nsThreadUtils.cpp
>+++ b/xpcom/glue/nsThreadUtils.cpp
>@@ -240,14 +240,61 @@ NS_ProcessNextEvent(nsIThread *thread, b
>     NS_ENSURE_TRUE(current, false);
>     thread = current.get();
>   }
> #endif
>   bool val;
>   return NS_SUCCEEDED(thread->ProcessNextEvent(mayWait, &val)) && val;
> }
> 
>+#ifndef XPCOM_GLUE_AVOID_NSPR
>+
>+class nsNameThreadRunnable : public nsIRunnable
>+{
>+public:
>+  nsNameThreadRunnable(const char * name) : mName(name) { }
>+
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIRUNNABLE
>+
>+protected:
>+  virtual ~nsNameThreadRunnable()
>+  {
>+  }
>+
>+  const char * mName;
>+};
>+
>+NS_IMPL_THREADSAFE_ISUPPORTS1(nsNameThreadRunnable, nsIRunnable)
>+
>+NS_IMETHODIMP
>+nsNameThreadRunnable::Run()
>+{
>+  PR_SetThreadName(mName);
>+  return NS_OK;
>+}
>+
>+void
>+NS_SetThreadName(nsIThread *thread, const char *name)
>+{
>+  if (!thread)
>+    return;
>+
>+  thread->Dispatch(new nsNameThreadRunnable(name),
>+                   nsIEventTarget::DISPATCH_NORMAL);
>+}
>+
>+#else // !XPCOM_GLUE_AVOID_NSPR
>+
>+void
>+NS_SetThreadName(nsIThread *thread, const char *name)
>+{
>+  // No NSPR, no love.
>+}
>+
>+#endif
>+
> #ifdef MOZILLA_INTERNAL_API
> nsIThread *
> NS_GetCurrentThread() {
>   return nsThreadManager::get()->GetCurrentThread();
> }
> #endif
>diff --git a/xpcom/glue/nsThreadUtils.h b/xpcom/glue/nsThreadUtils.h
>--- a/xpcom/glue/nsThreadUtils.h
>+++ b/xpcom/glue/nsThreadUtils.h
>@@ -204,16 +204,22 @@ NS_HasPendingEvents(nsIThread *thread = 
>  *
>  * @returns
>  *   A boolean value that if "true" indicates that an event from the current
>  *   thread's event queue was processed.
>  */
> extern NS_COM_GLUE bool
> NS_ProcessNextEvent(nsIThread *thread = nsnull, bool mayWait = true);
> 
>+/**
>+ * Set name of the target thread.  This operation is asynchronous.
>+ */
>+extern NS_COM_GLUE void
>+NS_SetThreadName(nsIThread *thread, const char *name);
>+
> //-----------------------------------------------------------------------------
> // Helpers that work with nsCOMPtr:
> 
> inline already_AddRefed<nsIThread>
> do_GetCurrentThread() {
>   nsIThread *thread = nsnull;
>   NS_GetCurrentThread(&thread);
>   return already_AddRefed<nsIThread>(thread);

Comment 41

[Honza Bambas (:mayhemer)]

Attachment: Part2 v2.2

- improved pool naming class
- NS_SetThreadName moved to this patch up from the part1

Comment 42

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 627269 [details] [diff] [review]
Part2 v2.2

Review of attachment 627269 [details] [diff] [review]:
-----------------------------------------------------------------

Consider adding and using this convenience function:

template <size_t LEN>
inline NS_METHOD
NS_NewNamedThread(const char name[LEN],
                  nsIThread **result,
                  nsIRunnable *initialEvent = nsnull,
                  PRUint32 stackSize = nsIThreadManager::DEFAULT_STACK_SIZE)
{
    MOZ_STATIC_ASSERT(LEN <= 16, "Thread name must be less than 16 characters");
    nsresult rv = NS_NewThread(result, initialEvent, stackSize);
    NS_SetThreadName(*result, NS_LITERAL_CSTRING(name));
    return rv;
}

This should catch, at compile time, most of the places where we're using too-long names.

Please file the follow-up bugs for these cases:
1. When we "Attach to Process" (on Windows), the thread names do not appear in the debugger thread window.
2. When the thread name is too long, we should truncate it instead of failing to set the thread name. For thread pools, we should truncate the thread name so that the thread number never gets cut off.

::: content/media/nsAudioStream.cpp
@@ +385,5 @@
>    if (!mAudioPlaybackThread) {
>      NS_NewThread(getter_AddRefs(mAudioPlaybackThread),
>                   nsnull,
>                   MEDIA_THREAD_STACK_SIZE);
> +    NS_SetThreadName(mAudioPlaybackThread, NS_LITERAL_CSTRING("Media Audio Stream"));

Use a shorter name (e.g. "Audio Playback" or "Audio Stream").

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +1646,5 @@
>      NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
>      return rv;
>    }
>  
> +  NS_SetThreadName(mDecodeThread, NS_LITERAL_CSTRING("Media Decode Thread"));

Do not need to include "Thread" in the thread names.

@@ +1673,5 @@
>        mState = DECODER_STATE_SHUTDOWN;
>        return rv;
>      }
> +
> +    NS_SetThreadName(mAudioThread, NS_LITERAL_CSTRING("Media Audio Thread"));

Ditto.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +235,5 @@
>  
>      // Make a lazy thread for any IO we need (like clearing or enumerating the
>      // contents of indexedDB database directories).
>      instance->mIOThread = new LazyIdleThread(DEFAULT_THREAD_TIMEOUT_MS,
> +                                             NS_LITERAL_CSTRING("Index DB I/O"),

s/Index DB/IndexedDB/

::: dom/indexedDB/TransactionThreadPool.cpp
@@ +136,5 @@
>    nsresult rv;
>    mThreadPool = do_CreateInstance(NS_THREADPOOL_CONTRACTID, &rv);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  rv = mThreadPool->SetName(NS_LITERAL_CSTRING("IndexDB Transaction"));

s/IndexDB/IndexedDB/

::: dom/plugins/base/android/ANPAudio.cpp
@@ +116,5 @@
>  
>  NS_IMETHODIMP
>  AudioRunnable::Run()
>  {
> +  PR_SetThreadName("Android Audio Thread");

s/ Thread//

::: dom/workers/RuntimeService.cpp
@@ +788,5 @@
>          NS_FAILED(priority->SetPriority(nsISupportsPriority::PRIORITY_LOW))) {
>        NS_WARNING("Could not lower the new thread's priority!");
>      }
> +
> +    NS_SetThreadName(thread, NS_LITERAL_CSTRING("Worker Thread"));

I suggest "DOM Worker"

::: netwerk/base/src/nsIOThreadPool.cpp
@@ +202,5 @@
>  nsIOThreadPool::ThreadFunc(void *arg)
>  {
>      nsIOThreadPool *pool = (nsIOThreadPool *) arg;
>  
> +    pool->mNaming("IO Thread");

It is OK to keep "Thread" here, considering the name of the class.

::: netwerk/base/src/nsSocketTransportService2.cpp
@@ +581,5 @@
>  
>  NS_IMETHODIMP
>  nsSocketTransportService::Run()
>  {
> +    PR_SetThreadName("Socket Transport Service");

Shorter name (e.g. "Socket Transport")

::: netwerk/base/src/nsStreamTransportService.cpp
@@ +442,5 @@
>      // Configure the pool
>      mPool->SetThreadLimit(4);
>      mPool->SetIdleThreadLimit(1);
>      mPool->SetIdleThreadTimeout(PR_SecondsToInterval(60));
> +    mPool->SetName(NS_LITERAL_CSTRING("Stream Transport Service"));

Shorter name (e.g. "Stream Transport").

::: netwerk/cache/nsCacheService.cpp
@@ +1152,5 @@
>      if (NS_FAILED(rv)) {
>          NS_RUNTIMEABORT("Can't create cache IO thread");
>      }
>  
> +    NS_SetThreadName(mCacheIOThread, NS_LITERAL_CSTRING("HTTP Cache"));

The cache isn't just for HTTP. For example, Thunderbird uses it for non-HTTP stuff. I suggest just "Cache IO", which matches "Cache Deleter."

::: parser/html/nsHtml5Module.cpp
@@ +147,5 @@
>  {
>    if (sOffMainThread) {
>      if (!sStreamParserThread) {
>        NS_NewThread(&sStreamParserThread);
> +      NS_SetThreadName(sStreamParserThread, NS_LITERAL_CSTRING("HTML5 Stream Parser"));

Shorter name (e.g. "HTML5 Parser")

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +194,5 @@
>  
>    (void) gCertVerificationThreadPool->SetIdleThreadLimit(5);
>    (void) gCertVerificationThreadPool->SetIdleThreadTimeout(30 * 1000);
>    (void) gCertVerificationThreadPool->SetThreadLimit(5);
> +  (void) gCertVerificationThreadPool->SetName(NS_LITERAL_CSTRING("SSL Cert Verification"));

shorter name (e.g. "SSL Cert").

::: security/manager/ssl/src/nsSmartCardMonitor.cpp
@@ +356,5 @@
>  
>  // C-like calling sequence to glue into PR_CreateThread.
>  void SmartCardMonitoringThread::LaunchExecute(void *arg)
>  {
> +  PR_SetThreadName("PSM SmartCard Monitoring");

Shorter name (e.g. "SmartCard").

::: storage/src/mozStorageConnection.cpp
@@ +561,5 @@
>        NS_WARNING("Failed to create async thread.");
>        return nsnull;
>      }
> +    static nsThreadPoolNaming naming;
> +    naming(NS_LITERAL_CSTRING("Storage Connection"), mAsyncExecutionThread);

shorter name (e.g. "Storage Conn.") so thread number doesn't get cut off.

::: xpcom/base/nsCycleCollector.cpp
@@ +3017,5 @@
>  
>  public:
>      NS_IMETHOD Run()
>      {
> +        PR_SetThreadName("XPCOM Cycle Collector");

shorter name (e.g. "Cycle Collector")

::: xpcom/glue/nsThreadUtils.cpp
@@ +226,5 @@
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIRUNNABLE
> +
> +protected:
> +  virtual ~nsNameThreadRunnable()

You do not need to declare a destructor, do you?

@@ +246,5 @@
> +void
> +NS_SetThreadName(nsIThread *thread, const nsACString &name)
> +{
> +  if (!thread) {
> +    PR_SetThreadName(name.BeginReading());

Do not set the name of the current thread if thread is NULL. Instead make it a no-op, as discussed on IRC.

::: xpcom/glue/nsThreadUtils.h
@@ +177,5 @@
>  extern NS_COM_GLUE bool
>  NS_ProcessNextEvent(nsIThread *thread = nsnull, bool mayWait = true);
>  
> +/**
> + * Set name of the target thread.  This operation is asynchronous.

Document the fact that long names will not work on some platforms (e.g. Linux) and that the thread name should be less than 16 characters. See also the note below.

Document that this is a no-op if thread is NULL.

@@ +180,5 @@
> +/**
> + * Set name of the target thread.  This operation is asynchronous.
> + */
> +extern NS_COM_GLUE void
> +NS_SetThreadName(nsIThread *thread, const nsACString &name);

Move the declaration to be under NS_NewThread.

@@ +448,5 @@
> +public:
> +  nsThreadPoolNaming() : mCounter(0) {}
> +
> +  /**
> +   * Creates and sets next thread name as "Pool Name #1"

s/"Pool Name #1"/"<aPoolName> #<n>"/

@@ +452,5 @@
> +   * Creates and sets next thread name as "Pool Name #1"
> +   * on the specified thread.  If no thread is specified (left
> +   * null) then the name is set on the current thread.
> +   */
> +  void operator()(const nsACString & aPoolName, nsIThread * aThread = nsnull);

Give this a searchable name.

::: xpcom/threads/nsIThreadPool.idl
@@ +73,5 @@
>    attribute nsIThreadPoolListener listener;
> +
> +  /**
> +   * Set label for threads in the pool.  All names are appended with
> +   * a numbering sufix like #1, #2.

"Set the label for threads in the pool. All threads will be named "<name> #<n>", where <n> is a serial number.

(Note that "suffix" is spelled wrong.)

::: xpcom/threads/nsProcessCommon.cpp
@@ +221,5 @@
>  #endif
>  
>  void PR_CALLBACK nsProcess::Monitor(void *arg)
>  {
> +    PR_SetThreadName("nsProcess::RunProcess");

Use a shorter name.

Comment 43

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

When I said "see also the note below," I meant "above," and I was referring to the NS_NewNamedThread template function.

Comment 44

[Wan-Teh Chang]

I opened NSPR bug 758837 for the NSPR patch.

Comment 45

[Honza Bambas (:mayhemer)]

Attachment: v3

Comment 46

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 627470 [details] [diff] [review]
v3

Review of attachment 627470 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +1661,5 @@
>                 "Should be on state machine or decode thread.");
>    mDecoder->GetReentrantMonitor().AssertCurrentThreadIn();
>    mStopAudioThread = false;
>    if (HasAudio() && !mAudioThread && !mAudioCaptured) {
> +    nsresult rv = NS_NewNamedThread("Audio Media",

Nit: "Media Audio" to be consistent with "Media Decode"?

::: netwerk/base/src/nsSocketTransportService2.cpp
@@ +581,5 @@
>  
>  NS_IMETHODIMP
>  nsSocketTransportService::Run()
>  {
> +    PR_SetThreadName("Socket Thread");

Nit: Do not need "Thread" in the name. Maybe "Socket Trans" to be consistent with "Stream Trans". or "STS" since that is what it is called in the log below.

::: netwerk/dns/nsHostResolver.cpp
@@ +955,5 @@
>  {
>      LOG(("nsHostResolver::ThreadFunc entering\n"));
> +
> +    static nsThreadPoolNaming naming;
> +    naming.SetThreadPoolName(NS_LITERAL_CSTRING("DNS Resolver"));

This will call SetThreadPoolName multiple times.

::: security/manager/ssl/src/nsPSMBackgroundThread.h
@@ +63,5 @@
>    // and/or exit is requested.
>    mozilla::CondVar mCond;
>  
> +  // The thread's name.
> +  nsCString mName;

Nit: private?

::: xpcom/glue/nsThreadUtils.cpp
@@ +214,5 @@
>  #endif
>    bool val;
>    return NS_SUCCEEDED(thread->ProcessNextEvent(mayWait, &val)) && val;
>  }
>  

namespace {

@@ +237,5 @@
> +{
> +  PR_SetThreadName(mName.BeginReading());
> +  return NS_OK;
> +}
> +

} // unnamed namespace

::: xpcom/glue/nsThreadUtils.h
@@ +45,5 @@
> +NS_SetThreadName(nsIThread *thread, const nsACString &name);
> +
> +/**
> + * Static length version of the above function checking length of the
> + * name at runtime.

s/runtime/compile time/.

@@ +52,5 @@
> +inline NS_COM_GLUE void
> +NS_SetThreadName(nsIThread *thread, const char (&name)[LEN])
> +{
> +  MOZ_STATIC_ASSERT(LEN <= 16,
> +                    "Thread name must be less than 16 characters");

s/less than/no more than/ (sorry)

@@ +483,5 @@
> +   * Creates and sets next thread name as "<aPoolName> #<n>"
> +   * on the specified thread.  If no thread is specified (left
> +   * null) then the name is set on the current thread.
> +   */
> +  void SetThreadPoolName(const nsACString & aPoolName,

s/left null/aThread is null/

Nit: IMO, it is better to make aThread explicit since you removed the defaulting to the current thread from the other functions. Up to you.

@@ +487,5 @@
> +  void SetThreadPoolName(const nsACString & aPoolName,
> +                         nsIThread * aThread = nsnull);
> +
> +private:
> +  PRInt32 mCounter;

Nit: use an unsigned type since this will never be negative?

Nit:
  nsThreadPoolNaming(const nsThreadPoolNaming &) MOZ_DELETE;
  void operator=(const nsThreadPoolNaming &) MOZ_DELETE;

::: xpcom/threads/LazyIdleThread.cpp
@@ +29,5 @@
>  
>  namespace mozilla {
>  
>  LazyIdleThread::LazyIdleThread(PRUint32 aIdleTimeoutMS,
> +                               const nsCSubstring& aName,

Nit: This must be nsCSubstring and not nsACString? not clear why.

Comment 47

[Honza Bambas (:mayhemer)]

(In reply to Brian Smith (:bsmith) from comment #46)
> ::: netwerk/base/src/nsSocketTransportService2.cpp
> @@ +581,5 @@
> >  
> >  NS_IMETHODIMP
> >  nsSocketTransportService::Run()
> >  {
> > +    PR_SetThreadName("Socket Thread");
> 
> Nit: Do not need "Thread" in the name. Maybe "Socket Trans" to be consistent
> with "Stream Trans". or "STS" since that is what it is called in the log
> below.

No.  Me and a lot of other people refer this thread as "The Socket Thread".  Also, I often mistake socket transport service and stream transport service.  When we rename this to Socket Transport then it could be also mistaken with Stream Transport.  So I'm against.

> 
> ::: netwerk/dns/nsHostResolver.cpp
> @@ +955,5 @@
> >  {
> >      LOG(("nsHostResolver::ThreadFunc entering\n"));
> > +
> > +    static nsThreadPoolNaming naming;
> > +    naming.SetThreadPoolName(NS_LITERAL_CSTRING("DNS Resolver"));
> 
> This will call SetThreadPoolName multiple times.

Not sure what you mean is an issue here.  nsHostResolver::ThreadFunc is an entry point for an always new thread.  So I think this is well places, unless I'm missing something.

> ::: xpcom/glue/nsThreadUtils.cpp
> @@ +214,5 @@
> >  #endif
> >    bool val;
> >    return NS_SUCCEEDED(thread->ProcessNextEvent(mayWait, &val)) && val;
> >  }
> >  
> 
> namespace {
> 
> @@ +237,5 @@
> > +{
> > +  PR_SetThreadName(mName.BeginReading());
> > +  return NS_OK;
> > +}
> > +
> 
> } // unnamed namespace
> 
> ::: xpcom/glue/nsThreadUtils.h
> @@ +45,5 @@
> > +NS_SetThreadName(nsIThread *thread, const nsACString &name);
> > +
> > +/**
> > + * Static length version of the above function checking length of the
> > + * name at runtime.
> 
> s/runtime/compile time/.
> 
> @@ +52,5 @@
> > +inline NS_COM_GLUE void
> > +NS_SetThreadName(nsIThread *thread, const char (&name)[LEN])
> > +{
> > +  MOZ_STATIC_ASSERT(LEN <= 16,
> > +                    "Thread name must be less than 16 characters");
> 
> s/less than/no more than/ (sorry)
> 
> @@ +483,5 @@
> > +   * Creates and sets next thread name as "<aPoolName> #<n>"
> > +   * on the specified thread.  If no thread is specified (left
> > +   * null) then the name is set on the current thread.
> > +   */
> > +  void SetThreadPoolName(const nsACString & aPoolName,
> 
> Nit: IMO, it is better to make aThread explicit since you removed the
> defaulting to the current thread from the other functions. Up to you.

No.  It is useful to not require it.  This is sometimes called from the thread entry function directly and a need to pass the current thread is unnecessary over-complication.

When you want to set name on the current thread directly, use PR_SetThreadName().  Therefor it was better to make thread ref required on NS_SetThreadName().

> 
> @@ +487,5 @@
> > +  void SetThreadPoolName(const nsACString & aPoolName,
> > +                         nsIThread * aThread = nsnull);
> > +
> > +private:
> > +  PRInt32 mCounter;
> 
> Nit: use an unsigned type since this will never be negative?

That would need cast when passed PR_AtomicIncrement.

> ::: xpcom/threads/LazyIdleThread.cpp
> @@ +29,5 @@
> >  
> >  namespace mozilla {
> >  
> >  LazyIdleThread::LazyIdleThread(PRUint32 aIdleTimeoutMS,
> > +                               const nsCSubstring& aName,
> 
> Nit: This must be nsCSubstring and not nsACString? not clear why.

I was ones told that non-XPCOM public API should use nsCSubstring& instead of nsACString&.  So I'm doing it.  For me it works well.

Comment 48

[Honza Bambas (:mayhemer)]

Attachment: v3.1

- bsmith's last review comments addressed
- changed use of PR_AtomicIncrement to just volatile ++
- asking for SR

Comment 49

[Benjamin Smedberg]

Comment on attachment 627927 [details] [diff] [review]
v3.1

glandium, are these static classes (nsThreadPoolNaming) a problem for static constructor counts?

Comment 50

[Mike Hommey [:glandium]]

Comment on attachment 627927 [details] [diff] [review]
v3.1

Review of attachment 627927 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #49)
> glandium, are these static classes (nsThreadPoolNaming) a problem for static
> constructor counts?

The static variables are fine, as they are defined inside functions. The members are a different story. They are fine if the class they are member doesn't have a global instance. OTOH, if that's the case, most likely, there's already a static initializer for these instances.

Comment 51

[Honza Bambas (:mayhemer)]

Attachment: v3.1 unbitrotted

Comment 52

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Can this land?

Comment 53

[Honza Bambas (:mayhemer)]

Comment on attachment 632274 [details] [diff] [review]
v3.1 unbitrotted

https://hg.mozilla.org/integration/mozilla-inbound/rev/772d9d20cdf9

Comment 54

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/772d9d20cdf9

Comment 55

[Octoploid]

This breaks the build (at least for me) on Linux:
...
/var/tmp/mozilla-central/js/src/jsgc.cpp:2690:5: error: use of undeclared identifier 'PR_SetCurrentThreadName'
    PR_SetCurrentThreadName("JS GC Helper");
    ^
1 error generated.

Comment 56

[(not currently active) Ted Mielczarek]

Are you building with system NSPR?

Presumably we need to bump our required NSPR version to 4.9.2.

Comment 57

[Octoploid]

(In reply to Ted Mielczarek [:ted] from comment #56)
> Are you building with system NSPR?

Yes.
 
> Presumably we need to bump our required NSPR version to 4.9.2.

I'm running 4.9.1.

Comment 58

[(not currently active) Ted Mielczarek]

Right, 4.9.2 has not been released yet, but that's the version containing these methods.

Comment 59

[Octoploid]

Understood. I will just build without system NSRP until the new version is released.

Comment 60

[John Tapsell]

Hi guys.

This change caused the process name to change from "firefox" to "Main Thread", as viewed from "top" etc.  Could you fix this please?

Comment 61

[Honza Bambas (:mayhemer)]

(In reply to John Tapsell from comment #60)
> Hi guys.
> 
> This change caused the process name to change from "firefox" to "Main
> Thread", as viewed from "top" etc.  Could you fix this please?

It's tracked in bug 765158 you are CC'ed on.  You don't need to push in another bug as well.

Comment 62

[Gregor Wagner [:gwagner]]

(In reply to Octoploid from comment #55)
> This breaks the build (at least for me) on Linux:
> ...
> /var/tmp/mozilla-central/js/src/jsgc.cpp:2690:5: error: use of undeclared
> identifier 'PR_SetCurrentThreadName'
>     PR_SetCurrentThreadName("JS GC Helper");
>     ^
> 1 error generated.

The same problem here. I can't build a threadsafe JS shell either. Is there any workaround?

Comment 63

[(not currently active) Ted Mielczarek]

Someone needs to bump the required version of NSPR to 4.9.2 in configure.