Closed Bug 1377344 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: nika, Assigned: nika)

References

Details

Attachments

(1 file, 1 obsolete file)

This could be really useful information for diagnosing what is causing hangs in telemetry, and many of our runnables are usefully labeled.
MozReview-Commit-ID: IYRHh6jiTeo
Attachment #8882442 - Flags: review?(nfroyd)
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.
Flags: needinfo?(dothayer)
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.
Flags: needinfo?(dothayer)
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.
Attachment #8882442 - Flags: review?(nfroyd) → review-
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
Attachment #8882617 - Flags: review?(nfroyd)
Attachment #8882617 - Flags: review?(n.nethercote)
Attachment #8882442 - Attachment is obsolete: true
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.
Attachment #8882617 - Flags: review?(nfroyd) → review+
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!
Attachment #8882617 - Flags: review?(n.nethercote) → review+
Pushed by michael@thelayzells.com:
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
See Also: → 1378280
See Also: 1378280
https://hg.mozilla.org/mozilla-central/rev/5bab7ca65592
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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.
Flags: needinfo?(dothayer)
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.
Flags: needinfo?(dothayer)
Depends on: 1380096
Duplicate of this bug: 1377241
You need to log in before you can comment on or make changes to this bug.