1362976 - Use SetThreadDescription in _PR_MD_SET_CURRENT_THREAD_NAME

Status: RESOLVED FIXED

(NSPR :: NSPR, enhancement)

Description

[Honza Bambas (:mayhemer)]

https://dxr.mozilla.org/mozilla-central/rev/17d8a1e278a9c54a6fdda9d390abce4077e55b20/nsprpub/pr/src/md/windows/ntthread.c#292

https://dxr.mozilla.org/mozilla-central/rev/17d8a1e278a9c54a6fdda9d390abce4077e55b20/nsprpub/pr/src/md/windows/w95thred.c#189

Comment 1

[Markus Stange [:mstange]]

Corresponding Chromium patch: https://codereview.chromium.org/2692213003/diff/80001/base/threading/platform_thread_win.cc

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Use SetThreadDescription in _PR_MD_SET_CURRENT_THREAD_NAME

Comment 3

[Makoto Kato [:m_kato]]

Comment on attachment 8884182 [details] [diff] [review]
Use SetThreadDescription in _PR_MD_SET_CURRENT_THREAD_NAME

Windows 10 build 1607 or later supports SetThreadDescription API to set thread name.  So we should use it if available.

Comment 4

[Kai Engert (:KaiE:)]

Comment on attachment 8884182 [details] [diff] [review]
Use SetThreadDescription in _PR_MD_SET_CURRENT_THREAD_NAME

I suggest that you check the result of the calls to MultiByteToWideChar.

https://msdn.microsoft.com/en-us/library/windows/desktop/dd319072(v=vs.85).aspx

Based on this API reference, there are several scenarios when MultiByteToWideChar may fail. If it failed, then we shouldn't use the wideName buffer and shouldn't call sSetThreadDescription.


You always return from the function, if sSetThreadDescription is set. The later code in this function contains some debugging hook, implemented using an exception. Could this currently be used by developers for debugging purposes? Unless you can explain why calling SetThreadDescription makes this debugging exception unnecessary, I'd prefer to keep it.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8884182 [details] [diff] [review]
Use SetThreadDescription in _PR_MD_SET_CURRENT_THREAD_NAME

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

::: pr/src/md/windows/w95thred.c
@@ +209,5 @@
> +    if (sSetThreadDescription) {
> +        WCHAR wideName[MAX_PATH];
> +        MultiByteToWideChar(CP_ACP, 0, name, -1, wideName, MAX_PATH);
> +        sSetThreadDescription(GetCurrentThread(), wideName);
> +        return;

Drive-by: Not all debuggers use GetThreadDescription, so I would suggest that you still fall-through to the old method instead of returning here.

Comment 6

[Honza Bambas (:mayhemer)]

+1 !

Comment 7

[(Away)]

I noticed that the Stylo code has SetThreadDescription("StyleThread#1") etc., and it gave me a very cool "CPU Usage (Attributed)" graph in xperf. It would be great to get this working for the rest of our threads.

m_kato are you still working on this?

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

+1, let's get this landed.

Comment 9

[Makoto Kato [:m_kato]]

(In reply to David Major [:dmajor] from comment #7)
> I noticed that the Stylo code has SetThreadDescription("StyleThread#1")
> etc., and it gave me a very cool "CPU Usage (Attributed)" graph in xperf. It
> would be great to get this working for the rest of our threads.
> 
> m_kato are you still working on this?

I cannot work this for next 2 weeks since I don't come at office.  (next 2 weeks are PTO and Austin).  If you want this soon, please pick up this.

Comment 10

[Kai Engert (:KaiE:)]

What's missing? Does the patch still require review?

Once it's reviewed, I can help to get it checked in.

Comment 11

[Honza Bambas (:mayhemer)]

I think only thing that really needs to be done is to remove the |return;| from inside the |if (sSetThreadDescription)| branch and use both ways of naming threads on Windows.

Comment 12

[(Away)]

And a return value check on MultiByteToWideChar for comment 4.

Comment 13

[(Away)]

Attachment: Use SetThreadDescription in _PR_MD_SET_CURRENT_THREAD_NAME

Rebased + check MultiByteToWideChar + don't return early

I haven't build or tested this. Is there a Try for NSPR?

Comment 14

[Kai Engert (:KaiE:)]

(In reply to David Major [:dmajor] from comment #13)
> Is there a Try for NSPR?

Not yet. See bug 1399095 for an idea.

Comment 15

[(Away)]

OK, I'll try on m-c instead: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8ad4f13d8ce0b4398138460bae607a71483d1e9

(Really this is small enough that I should just build and step through it locally, but my objdirs are in bad shape today)

Comment 16

[Kai Engert (:KaiE:)]

Comment on attachment 8934173 [details] [diff] [review]
Use SetThreadDescription in _PR_MD_SET_CURRENT_THREAD_NAME

I haven't worked with this code, and don't work on Windows.

Honza, could you review this code?

Comment 17

[Honza Bambas (:mayhemer)]

Comment on attachment 8934173 [details] [diff] [review]
Use SetThreadDescription in _PR_MD_SET_CURRENT_THREAD_NAME

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

thanks!

::: pr/src/md/windows/ntthread.c
@@ +311,4 @@
>  
> +   if (sSetThreadDescription) {
> +      WCHAR wideName[MAX_PATH];
> +      if (MultiByteToWideChar(CP_ACP, 0, name, -1, wideName, MAX_PATH)) {

Please use CP_UTF8 code page as a source coding.  I honestly don't think there will ever be above 7f chars in the name, but for consistency UTF-8 is preferred.

::: pr/src/md/windows/w95thred.c
@@ +1,1 @@
> +

remove this blank line

@@ +236,4 @@
>  
> +   if (sSetThreadDescription) {
> +      WCHAR wideName[MAX_PATH];
> +      if (MultiByteToWideChar(CP_ACP, 0, name, -1, wideName, MAX_PATH)) {

UTF8 as well here.

Comment 18

[Masatoshi Kimura [:emk]]

(In reply to Honza Bambas (:mayhemer) from comment #17)
> ::: pr/src/md/windows/ntthread.c
> @@ +311,4 @@
> >  
> > +   if (sSetThreadDescription) {
> > +      WCHAR wideName[MAX_PATH];
> > +      if (MultiByteToWideChar(CP_ACP, 0, name, -1, wideName, MAX_PATH)) {
> 
> Please use CP_UTF8 code page as a source coding.  I honestly don't think
> there will ever be above 7f chars in the name, but for consistency UTF-8 is
> preferred.

No, no, no! Windows multibyte API must use ANSI charset for consistency.
If you need Unicode strings, please add widechar API.

Comment 19

[Masatoshi Kimura [:emk]]

Especially, THREADNAME_INFO.szName expects ANSI charset.

Comment 20

[Kai Engert (:KaiE:)]

Honza, do you agree with comment 18 and 19 ?

Comment 21

[Honza Bambas (:mayhemer)]

(In reply to Masatoshi Kimura [:emk] from comment #18)
> (In reply to Honza Bambas (:mayhemer) from comment #17)
> > ::: pr/src/md/windows/ntthread.c
> > @@ +311,4 @@
> > >  
> > > +   if (sSetThreadDescription) {
> > > +      WCHAR wideName[MAX_PATH];
> > > +      if (MultiByteToWideChar(CP_ACP, 0, name, -1, wideName, MAX_PATH)) {
> > 
> > Please use CP_UTF8 code page as a source coding.  I honestly don't think
> > there will ever be above 7f chars in the name, but for consistency UTF-8 is
> > preferred.
> 
> No, no, no! Windows multibyte API must use ANSI charset for consistency.

That's the source conversion what I want you to change.  We control the thread name strings (as hard coded strings) and I would more expect them to be UTF-8 encoded then ANSI encoded.  char* can hold UTF-8 quit well.

> If you need Unicode strings, please add widechar API.

However, I think both ANSI and UTF-8 will work here, as no one will ever use UTF-8 to name a thread :)

Comment 22

[Masatoshi Kimura [:emk]]

(In reply to Honza Bambas (:mayhemer) from comment #21)
> That's the source conversion what I want you to change.  We control the
> thread name strings (as hard coded strings) and I would more expect them to
> be UTF-8 encoded then ANSI encoded.  char* can hold UTF-8 quit well.

Then, some NSPR char* functions will take ANSI strings and some others will take UTF-8? It would be a nightmare...
I'm fine if all NSPR char* APIs consistently take UTF-8.

> However, I think both ANSI and UTF-8 will work here, as no one will ever use
> UTF-8 to name a thread :)

If no one ever use non-ASCII chars anyway, what is the problem?

Comment 23

[Honza Bambas (:mayhemer)]

Go ahead with CP_ACP, if there are problems, we can change it relatively easily.

Comment 24

[(No longer employed by Mozilla) Aaron Klotz]

Kai, could you help land this, please?

Comment 25

[Kai Engert (:KaiE:)]

Sure, I can help. It wasn't obvious that the discussion has completed and that the patch is now ready for checkin.

Comment 26

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nspr/rev/ca06d0793318

I'll land this into m-i with a new nspr beta in bug 1420407.