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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1292600-Add-telemetry-to-measure-how-idle-budget.patch

Comment 2

[Andreas Farre [:farre]]

Attachment: 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.

Comment 3

[Nathan Froyd [:froydnj]]

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.

Comment 4

[Andreas Farre [:farre]]

(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

[Benjamin Smedberg]

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.

Comment 6

[Andreas Farre [:farre]]

Attachment: 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.

Comment 7

[Nathan Froyd [:froydnj]]

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.

Comment 8

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1292600-Add-telemetry-to-measure-how-idle-budget.patch

Yep, that variable should move, refactoring artifact. Carrying over r+.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5901d855faf
Add telemetry to measure how idle budgets are used. r=froydnj, data-r=bsmedberg

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e5901d855faf