Telemetry for mean-time-between-unlabeled-runnables

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
P2
normal
RESOLVED FIXED
9 months ago
6 months ago

People

(Reporter: billm, Assigned: billm)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

9 months ago
We should compute this.
Priority: -- → P2
(Assignee)

Updated

7 months ago
Flags: needinfo?(wmccloskey)
(Assignee)

Comment 1

7 months ago
Created attachment 8866112 [details] [diff] [review]
Make SchedulerGroup::Runnable QIable

This patch makes it possible to use QI to figure out if a runnable has been labeled with a SchedulerGroup.
Flags: needinfo?(wmccloskey)
Attachment #8866112 - Flags: review?(nfroyd)
(Assignee)

Comment 2

7 months ago
Created attachment 8866113 [details] [diff] [review]
compute mean time between unlabeled runnables

This does the telemetry for mean time between unlabeled runnables.
Attachment #8866113 - Flags: review?(nfroyd)
(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?
Flags: needinfo?(wmccloskey)
(Assignee)

Comment 4

7 months ago
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.
Flags: needinfo?(wmccloskey)
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.
Attachment #8866112 - Flags: review?(nfroyd) → review+
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": ["wmccloskey@mozilla.com"],
> +    "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.
Attachment #8866113 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 7

7 months ago
Created attachment 8866548 [details] [diff] [review]
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.
Attachment #8866113 - Attachment is obsolete: true
Attachment #8866548 - Flags: review?(benjamin)

Comment 8

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

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

Comment 9

7 months ago
Pushed by wmccloskey@mozilla.com:
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

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b1858bf27d48
https://hg.mozilla.org/mozilla-central/rev/3aa433b56b2d
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.