Closed Bug 1296878 Opened 3 years ago Closed 3 years ago

TraceLogger: Report thread names when available

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(3 files, 1 obsolete file)

When tracelogger lists out all of the threads to inspect, I'd really like to see thread names.

I have patches to the github tracelogger repo that implement the UI part. I'll clean them up and post them a bit later.
First, implement this where it's easy -- namely, when pthread_getname_np (or similar) exists. I'd like to also enable this for Windows by storing it in TLS, but that's going to require some further work. (Especially since I discovered that xpcom/glue is already doing this, and Firefox is setting names through that, and so I need to figure out what to use when. Nick, I'll probably be pestering you about that.)
Attachment #8783215 - Flags: review?(nfitzgerald)
Assuming the existence of ThisThread::GetName, record thread names in the json output of TraceLogger.
Attachment #8783217 - Flags: review?(hv1989)
Comment on attachment 8783217 [details] [diff] [review]
TraceLogger: report thread name when available

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

Cool! Thanks
Attachment #8783217 - Flags: review?(hv1989) → review+
Comment on attachment 8783215 [details] [diff] [review]
Implement js::ThisThread::GetName for limited set of platforms

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

::: js/src/threading/Thread.h
@@ +185,5 @@
>  // nothing.
>  void SetName(const char* name);
>  
> +// Get the current thread name. As with SetName, not available on all
> +// platforms. On these platforms getName() will give back an empty string.

Nit: What does "will give back an empty string" mean? Set the first byte of `nameBuffer` to nul? Without having read the implementation, it isn't crystal clear to me, so I assume it won't necessarily be to other potential API consumers either.

I'm assuming it will just put a nul in the first byte of the buffer, which means `len` is always greater than 0, so we should probably assert that. It seems good to assert against craziness.

::: js/src/threading/posix/Thread.cpp
@@ +179,5 @@
> +#elif defined(__NetBSD__)
> +  rv = pthread_getname_np(pthread_self(), nameBuffer, len);
> +#else
> +  rv = pthread_getname_np(pthread_self(), nameBuffer, len);
> +#endif

pthreads is a mess...

@@ +181,5 @@
> +#else
> +  rv = pthread_getname_np(pthread_self(), nameBuffer, len);
> +#endif
> +
> +  MOZ_RELEASE_ASSERT(len > 0);

Nice, here it is.
Attachment #8783215 - Flags: review?(nfitzgerald) → review+
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfbac36772eb
Implement js::ThisThread::GetName for limited set of platforms, r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/05f3d17beffe
TraceLogger: report thread name when available, r=h4writer
Backout by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fab74549bba
Backed out 5 changesets (bug 1296878, bug 1296876, bug 1296875) for compilation failures, a=CLOSED TREE
This doesn't cover the full range of thread name accessing functions (the existence of the function doesn't tell you whether it's a 3-arg, 2-arg, or 1-arg version, and some platforms have pthread_get_name_np). But it turns out that this check plus a little bit of per-platform testing in the source gets us everything we need for our tier 1 platforms.
Attachment #8784065 - Flags: review?(mh+mozilla)
Comment on attachment 8784065 [details] [diff] [review]
configure probe for pthread_getname_np

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

::: js/src/old-configure.in
@@ +1217,5 @@
>  dnl Checks for library functions.
>  dnl ========================================================
>  AC_PROG_GCC_TRADITIONAL
>  AC_FUNC_MEMCMP
> +AC_CHECK_FUNCS([getc_unlocked _getc_nolock gmtime_r localtime_r pthread_getname_np])

We're not checking for pthread_setname_np, I don't see why we need to check for pthread_getname_np. Use the same preprocessor logic as for pthread_setname_np.
Attachment #8784065 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #8)
> Comment on attachment 8784065 [details] [diff] [review]
> configure probe for pthread_getname_np
> 
> Review of attachment 8784065 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/old-configure.in
> @@ +1217,5 @@
> >  dnl Checks for library functions.
> >  dnl ========================================================
> >  AC_PROG_GCC_TRADITIONAL
> >  AC_FUNC_MEMCMP
> > +AC_CHECK_FUNCS([getc_unlocked _getc_nolock gmtime_r localtime_r pthread_getname_np])
> 
> We're not checking for pthread_setname_np, I don't see why we need to check
> for pthread_getname_np. Use the same preprocessor logic as for
> pthread_setname_np.

... which you're apparently already doing...

Also note that AC_CHECK_FUNCS doesn't fail when the function is missing (thankfully, otherwise this would break Windows builds), and sets HAVE_UPPERCASE_OF_THE_FUNCTION. It doesn't look like your patches are using HAVE_PTHREAD_GETNAME_NP.
(In reply to Mike Hommey [:glandium] from comment #9)
> (In reply to Mike Hommey [:glandium] from comment #8)
> > Comment on attachment 8784065 [details] [diff] [review]
> > configure probe for pthread_getname_np
> > 
> > Review of attachment 8784065 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/src/old-configure.in
> > @@ +1217,5 @@
> > >  dnl Checks for library functions.
> > >  dnl ========================================================
> > >  AC_PROG_GCC_TRADITIONAL
> > >  AC_FUNC_MEMCMP
> > > +AC_CHECK_FUNCS([getc_unlocked _getc_nolock gmtime_r localtime_r pthread_getname_np])
> > 
> > We're not checking for pthread_setname_np, I don't see why we need to check
> > for pthread_getname_np. Use the same preprocessor logic as for
> > pthread_setname_np.
> 
> ... which you're apparently already doing...
> 
> Also note that AC_CHECK_FUNCS doesn't fail when the function is missing
> (thankfully, otherwise this would break Windows builds), and sets
> HAVE_UPPERCASE_OF_THE_FUNCTION. It doesn't look like your patches are using
> HAVE_PTHREAD_GETNAME_NP.

I'm not sure what you're asking for here. I guess I should have uploaded my current patch here, since it looks like you're basing this review on it.

The logic you are looking at is broken, which is why I didn't just land with it. It turns out that the situation with getname is more complicated than setname. setname exists more places than getname does. On Android, for example, I have to use prctl(PR_GET_NAME), but pthread_setname_np works.

The logic is simpler and actually correct if I use HAVE_PTHREAD_GETNAME_NP. Using the nest of platform-specific defines strikes me as rather awful in the first place, since that's what configure is for (and as ugly as configure may be, it does work for that purpose.) In the getname case, I can't *just* use the configure check, but it gets me 90% of the way there.

How about this: fitzgen already r+'ed the patch that adds ThisThread::GetName, so I'll take that as an acceptance of adding the function. I'll fold the two together now and ask you for review of the combination, just for the portion containing the configure check and the code that uses it.

I'm somewhat tempted to add a pthread_setname_np check, since the platform-specific checks bother me, but given that it's already there and working, I'd rather leave it alone. (Unless you tell me to do otherwise in your review.)
Combined patch.
Attachment #8784922 - Flags: review?(mh+mozilla)
Attachment #8784065 - Attachment is obsolete: true
Comment on attachment 8784922 [details] [diff] [review]
Implement js::ThisThread::GetName for limited set of platforms

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

Funny thing: NSPR uses pthread_setname_np, but not pthread_getname_np: it keeps the name in its own PRThread struct.
Attachment #8784922 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #12)
> Comment on attachment 8784922 [details] [diff] [review]
> Implement js::ThisThread::GetName for limited set of platforms
> 
> Review of attachment 8784922 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Funny thing: NSPR uses pthread_setname_np, but not pthread_getname_np: it
> keeps the name in its own PRThread struct.

Yeah, it kind of has to, because Windows doesn't have thread names. It can fake something when the debugger is attached, but there's still no way to read it. See bug 1297215. I was planning on doing the same thing in js/src/threading for Windows, but I didn't want to duplicate what was already being done, especially since I wouldn't see the names set via NSPR that way. If we decide we really need them, and we want to eliminate NSPR from SpiderMonkey, we'll have to call both setters or something.
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09b66fdcbc58
Implement js::ThisThread::GetName for limited set of platforms, r=fitzgen,glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/22e96c9d3b0a
TraceLogger: report thread name when available, r=h4writer
https://hg.mozilla.org/mozilla-central/rev/09b66fdcbc58
https://hg.mozilla.org/mozilla-central/rev/22e96c9d3b0a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1298451
Duplicate of this bug: 944702
You need to log in before you can comment on or make changes to this bug.