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

(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 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.

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

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

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