1351021 - Telemetry for mean-time-between-unlabeled-runnables

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We should compute this.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Make SchedulerGroup::Runnable QIable

This patch makes it possible to use QI to figure out if a runnable has been labeled with a SchedulerGroup.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: compute mean time between unlabeled runnables

This does the telemetry for mean time between unlabeled runnables.

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bill McCloskey (:billm) from comment #2)
> Created attachment 8866113 [details] [diff] [review]
> compute mean time between unlabeled runnables
> 
> This does the telemetry for mean time between unlabeled runnables.

This patch marks the dequeud time of an unlabeled runnable instead of the enqueued one.
IIRC, in the cooperative thread approach, we have to run all the runnables arrive earlier then this unlabeled one(and, somehow, have to disable the preemption) before running this unlabeled runnable.
Will the enqueued time be better then the dequeued time for further analysis?

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I thought about this last night. I think it depends on whether the unlabeled runnable is actually for the foreground tab or for a background tab.

If it's for a background tab, then the negative impact of not labeling the runnable happens when it actually runs. During that period, we're not able to run any foreground events.

If the unlabeled event is for a foreground tab, then the negative impact of not labeling the runnable happens while the runnable is in the queue. During this time, the scheduler might have run the runnable sooner if it knew it was for a foreground tab.

Since we don't actually know which of these cases is true, it's hard to know what the right thing to measure is. I'd rather go with the current patch since it's easier to measure. Everything happens on the main thread.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8866112 [details] [diff] [review]
Make SchedulerGroup::Runnable QIable

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

::: xpcom/threads/SchedulerGroup.cpp
@@ +337,5 @@
> +    if (named) {
> +      named->GetName(aName);
> +    }
> +  }
> +  aName.AppendASCII("(labeled)");

This is a little weird; if mRunnable *doesn't* have a name, and doesn't implement nsINamed, we'll end up with a name of "(labeled)".  Is that the intended behavior?  I guess this is just copying behavior that previously existed, but it still seems unusual.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8866113 [details] [diff] [review]
compute mean time between unlabeled runnables

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

The commit message seems incomplete.  See also comments below about the commit message.

r=me, but please ask for that data collection review.

::: toolkit/components/telemetry/Histograms.json
@@ +11493,5 @@
>      "description": "Time (ms) for the APZ handled wheel event spent in handlers."
> +  },
> +  "TIME_BETWEEN_UNLABELED_RUNNABLES_MS": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "60",

Does this need a data collection review?  https://wiki.mozilla.org/Firefox/Data_Collection#Requesting_Approval suggests yes.

::: xpcom/threads/nsThread.cpp
@@ +1184,5 @@
>      GetIdleEvent(aEvent, aProofOfLock);
> +
> +    if (*aEvent && aPriority) {
> +      // Idle events count as normal priority.
> +      *aPriority = nsIRunnablePriority::PRIORITY_NORMAL;

This is sort of unfortunate, given that we're starting to emphasize idle runnables as a separate concept from normal runnables.  But maybe they're equally interesting for this purpose?

@@ +1271,5 @@
>  
>  #ifndef RELEASE_OR_BETA
>          nsCString name;
> +        bool labeled = false;
> +        if (RefPtr<SchedulerGroup::Runnable> groupRunnable = do_QueryObject(event)) {

The previous patch cites its justification as being able to use QI here, but instead we're using QueryObject...we're doing this because SchedulerGroup::Runable isn't really an interface?

@@ +1284,5 @@
> +          name.AssignLiteral("anonymous runnable");
> +        }
> +
> +        // High-priority runnables are ignored here since they'll run right away
> +        // even with the cooperative scheduler.

So we're collecting this telemetry to figure out how much we might win with the cooperative scheduler?  We should make that more explicit in the commit message.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: compute mean time between unlabeled runnables

Asking for data review on this.

> This is sort of unfortunate, given that we're starting to emphasize idle
> runnables as a separate concept from normal runnables.  But maybe they're
> equally interesting for this purpose?

They are equally interesting. I did consider adding a LOW_PRIORITY constant, but then I would actually have to implement code to do something with it in Dispatch. And I didn't want to have to do that.

> The previous patch cites its justification as being able to use QI here, but
> instead we're using QueryObject...we're doing this because
> SchedulerGroup::Runable isn't really an interface?

Yeah, the advice from https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/nsCOMPtr_versus_RefPtr suggests that RefPtr should be used for concrete classes like SchedulerGroup::Runnable. And I don't think do_QueryInterface works for nsCOMPtrs.

Comment 8

[Benjamin Smedberg]

Comment on attachment 8866548 [details] [diff] [review]
compute mean time between unlabeled runnables

data-r=me - I did not review the code

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1858bf27d48
Make SchedulerGroup::Runnable QIable (r=froydnj)
https://hg.mozilla.org/integration/mozilla-inbound/rev/3aa433b56b2d
Compute mean time between unlabeled (r=froydnj,data-r=bsmedberg)

Comment 10

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/b1858bf27d48
https://hg.mozilla.org/mozilla-central/rev/3aa433b56b2d