1377344 - Record the name of the currently running Runnable on thread hangs for BHR

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nika Layzell [:nika] (ni? for response)]

This could be really useful information for diagnosing what is causing hangs in telemetry, and many of our runnables are usefully labeled.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Record the name of the currently running Runnable on thread hangs for BHR

MozReview-Commit-ID: IYRHh6jiTeo

Comment 2

[Nika Layzell [:nika] (ni? for response)]

dthayer, do you have any ideas for how we would want to expose this in hangs.html? I was thinking it might be nice to select a single runnable and see the stacks/hangs which are caused by that runnable.

Comment 3

[Alex Thayer [:alexical] (she/her)]

That sounds good. We could do something where we show a list of runnables ranked by total hang time under that runnable, and then allow filtering the tree by a given runnable.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8882442 [details] [diff] [review]
Record the name of the currently running Runnable on thread hangs for BHR

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

Unsure of the right resolution for the GetStacksInternal issue below, so r-.

::: tools/profiler/public/GeckoProfiler.h
@@ +258,2 @@
>  // the native stack's program counters and length as two arguments if
>  // aSampleNative is true.

This documentation needs updating for the new argument you added to the callback, I believe.

::: xpcom/threads/ThreadStackHelper.cpp
@@ +118,2 @@
>  {
> +  aRunnableName.SetIsVoid(true);

Can we please not use void strings here?  You don't check void-ness anywhere else in this code, and thinking that void-ness implies empty-string-ness is not a great assumption.  If you want to differentiate between "the runnable doesn't provide a name" and "the runnable has an empty name", maybe we should use a `Maybe`?  Otherwise, we should just use `.Truncate()` here.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Record the name of the currently running Runnable on thread hangs for BHR, r=njn

I was originally thinking of distinguishing between doesn't provide and has an
empty name, but after I wrote this I realized that didn't make much sense. I
think I'll change this to `.Truncate()`.

I'm also going to ask nick to review the changes to the profiler, as I imagine
he might have opinions as to better ways to handle the change I'm making.

MozReview-Commit-ID: IYRHh6jiTeo

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8882617 [details] [diff] [review]
Record the name of the currently running Runnable on thread hangs for BHR, r=njn

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

This all seems reasonable to me, but I am happy to listen to njn's feedback as well.

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 8882617 [details] [diff] [review]
Record the name of the currently running Runnable on thread hangs for BHR, r=njn

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

Something like "???" is a reasonable choice when you can't get a name, at leastin part because it can be useful to distinguish "no name" from "empty name".

I suggest |aRunnableName = "???";| instead of |aRunnableName.Truncate();|, because I think it *is* valuable to distinguish "no name obtained" from "empty name".

::: toolkit/components/telemetry/ThreadHangStats.h
@@ +187,3 @@
>  
>  public:
> +  explicit HangHistogram(HangStack&& aStack, nsACString&& aRunnableName)

|nsACString&&| is non-standard; I can't find any other such occurrences in the codebase. |const nsACString&| is normal.

::: tools/profiler/public/GeckoProfiler.h
@@ +271,2 @@
>                                         aCallback,
>                                       bool aSampleNative = true))

Can you introduce a typedef for the function? Perhaps "StackCallback"? You could put the argument names into the typedef that way, and you could use the typedef in platform.cpp too.

::: xpcom/threads/ThreadStackHelper.cpp
@@ +147,5 @@
> +    if (aIsMainThread) {
> +      strncpy(nameBuffer, nsThread::sMainThreadRunnableName, sizeof(nameBuffer));
> +      // Make sure the string is null-terminated.
> +      nameBuffer[sizeof(nameBuffer) - 1] = '\0';
> +    }

Do Runnables only run on the main thread? A brief comment explaining this |aIsMainThread| condition would be helpful.

::: xpcom/threads/nsThread.cpp
@@ +1419,5 @@
>            idleTimer.emplace(name, mNextIdleDeadline);
>          }
>        }
> +
> +      // Set the sRunnableName static to the name of the

Truncated comment!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bab7ca65592
Record the name of the currently running Runnable on thread hangs for BHR, r=njn, r=froydnj

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5bab7ca65592

Comment 10

[Nika Layzell [:nika] (ni? for response)]

dthayer, do we expose this yet in hangs.html / do you have any ideas for how we should expose it? 

I think that as a nice starting point it would be nice to have 2 things:
1. A sorted list of the most frequently hanging runnables, and
2. A way to filter hangs based on runnable name.

Comment 11

[Alex Thayer [:alexical] (she/her)]

We don't right now. I'll add an issue for it. Should be pretty simple, I'll just add a tab for it, and it will work roughly like Categories does right now.