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

RESOLVED FIXED in Firefox 56

Status

()

Core
XPCOM
RESOLVED FIXED
10 months ago
8 months ago

People

(Reporter: Nika, Assigned: Nika)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

10 months ago
Created attachment 8882442 [details] [diff] [review]
Record the name of the currently running Runnable on thread hangs for BHR

MozReview-Commit-ID: IYRHh6jiTeo
Attachment #8882442 - Flags: review?(nfroyd)
(Assignee)

Comment 2

10 months ago
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.
(Assignee)

Updated

10 months ago
Flags: needinfo?(dothayer)

Comment 3

10 months ago
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-
(Assignee)

Comment 5

10 months ago
Created attachment 8882617 [details] [diff] [review]
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
Attachment #8882617 - Flags: review?(nfroyd)
Attachment #8882617 - Flags: review?(n.nethercote)
(Assignee)

Updated

10 months ago
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+

Comment 8

10 months ago
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

Comment 9

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5bab7ca65592
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Comment 10

10 months ago
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)

Comment 11

10 months ago
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)
(Assignee)

Updated

10 months ago
Depends on: 1380096
(Assignee)

Updated

8 months ago
Blocks: 1393581
(Assignee)

Updated

8 months ago
Duplicate of this bug: 1377241
You need to log in before you can comment on or make changes to this bug.