The attached patch allows adding thread names to the log when using "threadnames" log module. With NSPR_LOG_MODULES=timestamp,threadnames,nsHttp:5 the log looks like: 2013-06-22 01:41:10.440000 UTC - [Main Thread] 0[22d660]: nsHttpChannel::OpenCacheEntry [this=3e1f378] 2013-06-22 01:41:10.440000 UTC - [Socket Thread] 6132[6d6cd8]: nsHttpConnectionMgr::AtActiveConnectionLimit [ci=....localhost:4444 caps=1]

any reason not to have this on by default? If there are projects that are relying on the exact format of the existing log, we could support a 'nothreads' option.

I'm not against, but someone from NSPR should decide. I personally am used to add the "system" modules - sync and timestamp. It's always helpful. threadnames will just be another one ;)

wtc, ping?

Comment on attachment 766208 [details] [diff] [review] v1 The code we use now has moved to xpcom. I will update the patch, and I think it will have more attention now ;)

This is very handy!

Comment on attachment 8713854 [details] [diff] [review] v2 Review of attachment 8713854 [details] [diff] [review]: ----------------------------------------------------------------- Yes! I like this idea, now we're down to just haggling over details. ::: xpcom/base/Logging.cpp @@ +183,3 @@ > if (!mAddTimestamp) { > fprintf_stderr(out, > + "[%p %s]: %s/%s %s%s", I don't think there's much point in printing the pointer for named threads, where it's just clutter; it's still useful for un-named threads, though. (I guess we could have issues with thread names not being unique due to shortening and therefore two different threads would print identical PR_GetThreadName(s) in the log? I would prefer to not clutter the logs for a corner case like that...though I think the JS engine has identically-named threads in its thread pool...) WDYT about changing the above to be: char name[100]; if (!currentThreadName) { snprintf_literal(name, "(unnamed thread %p)", currentThread); currentThreadName = name; } and then just printing currentThreadName to the logs?

And here it is!

Comment on attachment 8714815 [details] [diff] [review] v2.1 Review of attachment 8714815 [details] [diff] [review]: ----------------------------------------------------------------- This is great! r=me with optional change below. ::: xpcom/base/Logging.cpp @@ +217,5 @@ > private: > OffTheBooksMutex mModulesLock; > nsClassHashtable<nsCharPtrHashKey, LogModule> mModules; > ScopedCloseFile mOutFile; > + PRThread *mMainThread; Might be worth a comment somewhere about how the thread manager isn't initialized when we're constructing LogModuleManager, hence the PRThread usage here.

(In reply to Nathan Froyd [:froydnj] from comment #9) > Might be worth a comment somewhere about how the thread manager isn't > initialized when we're constructing LogModuleManager, hence the PRThread > usage here. I'm not sure what you mean. I've added this just to give the main thread a name on windows where the main thread doesn't have the name by default. We could also give the main thread a name first time we initialize NSPR (soon in main()). But that doesn't seem right to me, as the default main thread name on some platforms may have a meaning. And I don't want to give the main thread our name just on win.

(In reply to Honza Bambas (:mayhemer) from comment #10) > (In reply to Nathan Froyd [:froydnj] from comment #9) > > Might be worth a comment somewhere about how the thread manager isn't > > initialized when we're constructing LogModuleManager, hence the PRThread > > usage here. > > I'm not sure what you mean. I meant that we'd typically use nsCOMPtr<nsIThread> for identifying threads, and using PRThread* is a bit unusual. But using PRThread* is also slightly more efficient in this context, I guess, and there's.... > I've added this just to give the main thread a name on windows where the > main thread doesn't have the name by default. We could also give the main > thread a name first time we initialize NSPR (soon in main()). But that > doesn't seem right to me, as the default main thread name on some platforms > may have a meaning. And I don't want to give the main thread our name just > on win. ...this--I didn't know about the subtleties of main thread naming here! Your logic seems reasonable.

(In reply to Nathan Froyd [:froydnj] from comment #11) > (In reply to Honza Bambas (:mayhemer) from comment #10) > > (In reply to Nathan Froyd [:froydnj] from comment #9) > > > Might be worth a comment somewhere about how the thread manager isn't > > > initialized when we're constructing LogModuleManager, hence the PRThread > > > usage here. > > > > I'm not sure what you mean. > > I meant that we'd typically use nsCOMPtr<nsIThread> for identifying threads, > and using PRThread* is a bit unusual. But using PRThread* is also slightly > more efficient in this context, I guess, and there's.... As we are working with PRThreads in the logging engine itself (since ever, actually) I don't think we need to special-comment on using it again on a different place in the same class. > > > I've added this just to give the main thread a name on windows where the > > main thread doesn't have the name by default. We could also give the main > > thread a name first time we initialize NSPR (soon in main()). But that > > doesn't seem right to me, as the default main thread name on some platforms > > may have a meaning. And I don't want to give the main thread our name just > > on win. > > ...this--I didn't know about the subtleties of main thread naming here! > Your logic seems reasonable. Yep, windows is a bitch :) Going to land as is.