Add telemetry to see if idle callbacks take more than their budgeted time

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: overholt, Assigned: farre)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

a year ago
Created attachment 8872414 [details] [diff] [review]
0001-Bug-1292600-Add-telemetry-to-measure-how-idle-budget.patch
(Assignee)

Updated

a year ago
Attachment #8872414 - Flags: review?(nfroyd)
Attachment #8872414 - Flags: feedback?
(Assignee)

Updated

a year ago
Attachment #8872414 - Flags: feedback?
(Assignee)

Comment 2

a year ago
Created attachment 8872433 [details] [diff] [review]
0001-Bug-1292600-Add-telemetry-to-measure-how-idle-budget.patch

Realised that we need to setup the AutoTimer with the mNexIdleDeadline before running the event, since that might actually clear it because of nesting.
Attachment #8872414 - Attachment is obsolete: true
Attachment #8872414 - Flags: review?(nfroyd)
Attachment #8872433 - Flags: review?(nfroyd)
Comment on attachment 8872433 [details] [diff] [review]
0001-Bug-1292600-Add-telemetry-to-measure-how-idle-budget.patch

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

This needs data review on the Histograms change.  I'll let Benjamin do that change; a few non-histogram related things need fixing.

Why do we care about this number, anyway?  What action do we hope to take as a result of the data we collect?

::: xpcom/threads/nsThread.cpp
@@ +1233,5 @@
>  }
>  
> +#ifndef RELEASE_OR_BETA
> +bool
> +GetLabeledRunnableName(nsIRunnable* aEvent, nsACString& aName)

Nit: static bool, please.

@@ +1326,5 @@
>          HangMonitor::NotifyActivity();
>  
>  #ifndef RELEASE_OR_BETA
>          nsCString name;
> +        bool labeled = GetLabeledRunnableName(event, name);

Maybe we should just hoist this call out...

@@ +1345,5 @@
> +      // If mNextIdleDeadline is not null, then event is a runnable
> +      // from the idle queue
> +      if (mNextIdleDeadline) {
> +        nsCString name;
> +        Unused << GetLabeledRunnableName(event, name);

...so we don't have to do redundant work on the main thread for idle runnables?

@@ +1349,5 @@
> +        Unused << GetLabeledRunnableName(event, name);
> +        // If we construct the AutoTimer with the deadline, then we'll
> +        // compute TimeStamp::Now() - mNextIdleDeadline when
> +        // accumulating telemetry.  If that is positive we've
> +        // overdrawn out idle budget, if it's negative it will go in

Nit: overdrawn *our* idle budget.

::: xpcom/threads/nsThread.h
@@ +279,5 @@
>    // Set to true if this thread creates a JSRuntime.
>    bool mCanInvokeJS;
> +
> +#ifndef RELEASE_OR_BETA
> +  mozilla::TimeStamp mNextIdleDeadline;

Using a member variable to pass this state around is ugh, but I'm not sure I have a better idea.
Attachment #8872433 - Flags: review?(nfroyd)
Attachment #8872433 - Flags: review?(benjamin)
Attachment #8872433 - Flags: feedback+
(Assignee)

Comment 4

a year ago
(In reply to Nathan Froyd [:froydnj] from comment #3)
> Comment on attachment 8872433 [details] [diff] [review]
> 0001-Bug-1292600-Add-telemetry-to-measure-how-idle-budget.patch
> 
> Review of attachment 8872433 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why do we care about this number, anyway?  What action do we hope to take as
> a result of the data we collect?
> 

Now that we're starting to add more usage of idleDispatch, especially without using the deadline from SetDeadline I want to be preemptive and track runnables that doesn't adhere to that deadline.

Comment 5

a year ago
Comment on attachment 8872433 [details] [diff] [review]
0001-Bug-1292600-Add-telemetry-to-measure-how-idle-budget.patch

data-r=me. I did not review the code.
Attachment #8872433 - Flags: review?(benjamin) → review+
(Assignee)

Comment 6

a year ago
Created attachment 8874423 [details] [diff] [review]
0001-Bug-1292600-Add-telemetry-to-measure-how-idle-budget.patch

Did some juggling with GetLabeledRunnableName to eliminate possible extra call.

I don't like mNextIdleDeadline either, but also have no better solution :(

Now the telemetry code is a bit more non-interleavy, so if we ever get a better idea it should be easy to rip out.
Attachment #8872433 - Attachment is obsolete: true
Attachment #8874423 - Flags: review?(nfroyd)
Comment on attachment 8874423 [details] [diff] [review]
0001-Bug-1292600-Add-telemetry-to-measure-how-idle-budget.patch

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

::: xpcom/threads/nsThread.cpp
@@ +1325,5 @@
>  
>  #ifndef RELEASE_OR_BETA
> +      Maybe<Telemetry::AutoTimer<Telemetry::MAIN_THREAD_RUNNABLE_MS>> timer;
> +      Maybe<Telemetry::AutoTimer<Telemetry::IDLE_RUNNABLE_BUDGET_OVERUSE_MS>> idleTimer;
> +      bool labeled = false;

Can we move this declaration to...

@@ +1332,2 @@
>          nsCString name;
> +        labeled = GetLabeledRunnableName(event, name);

...here?  It might be my limited review context, but declaring variables as close as possible to their uses is a good thing, and I can't see that labeled is used outside this block.
Attachment #8874423 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 8

a year ago
Created attachment 8875284 [details] [diff] [review]
0001-Bug-1292600-Add-telemetry-to-measure-how-idle-budget.patch

Yep, that variable should move, refactoring artifact. Carrying over r+.
Attachment #8874423 - Attachment is obsolete: true
Attachment #8875284 - Flags: review+

Comment 9

a year ago
Pushed by afarre@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5901d855faf
Add telemetry to measure how idle budgets are used. r=froydnj, data-r=bsmedberg
https://hg.mozilla.org/mozilla-central/rev/e5901d855faf
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: affected → ---
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1371440
Depends on: 1382701
You need to log in before you can comment on or make changes to this bug.