Closed Bug 1267904 Opened 4 years ago Closed 4 years ago

Add telemetry for WorkerMainThreadRunnable

Categories

(Core :: DOM: Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: ehsan, Assigned: baku)

Details

(Whiteboard: [tw-dom] btpp-active)

Attachments

(1 file)

We need to start collecting telemetry for MainThreadWorkerControlRunnable to measure how long various things that we proxy to the main thread take in the wild.  Based on this we can get information about what cases are real world performance issues that we need to fix.

I'm imagining that we'd make the consumer of MainThreadWorkerControlRunnable pass in an identifier of some sort so that we can differentiate between the telemetry histograms for all of the consumers, both existing ones and the ones that we add in the future.  The ID can just be the histogram ID.
Whiteboard: [tw-dom]
I don't know what you think you are measuring but MainThreadWorkerControlRunnable is almost certainly not what you want.
The goal is to measure how long it takes to proxy work to the main thread and get the results back on the worker side.  Baku suggested this class, if this isn't the right place can you please update the summary to make it point to the right thing?  Thanks!
Well there's not just one class to look at ...  But WorkerControlRunnables are used for bookkeeping, not script observable functionality, and MainThreadWorkerControlRunnable is just a WorkerControlRunnable that can be dispatched from the main thread (instead of the parent thread).

I'd start by looking at every time we spin up a sync loop, but there is probably more than just that do be done.
Attached patch tele.patchSplinter Review
Assignee: nobody → amarchesini
Attachment #8746172 - Flags: review?(khuey)
Whiteboard: [tw-dom] → [tw-dom] btpp-active
Summary: Add telemetry for MainThreadWorkerControlRunnable → Add telemetry for WorkerMainThreadRunnable
Comment on attachment 8746172 [details] [diff] [review]
tele.patch

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

::: dom/workers/WorkerRunnable.cpp
@@ +585,5 @@
>  WorkerMainThreadRunnable::Dispatch(ErrorResult& aRv)
>  {
>    mWorkerPrivate->AssertIsOnWorkerThread();
>  
> +  TimeStamp startTime = TimeStamp::Now();

NowLoRes?

::: dom/workers/WorkerRunnable.h
@@ +444,5 @@
>  
>  private:
>    NS_IMETHOD Run() override;
> +
> +  const nsCString mTelemetryKey;

Place this with the other member variables above please.

@@ +464,1 @@
>    {}

Please move this ctor out of line into WorkerRunnable.cpp

::: toolkit/components/telemetry/Histograms.json
@@ +10594,1 @@
>    }

I don't know enough about this to know whether this is right or not ... but it looks reasonable ...

Please add khuey@mozilla.com to the alert_emails.
Attachment #8746172 - Flags: review?(khuey) → review+
https://hg.mozilla.org/mozilla-central/rev/08f3c7a9f5a8
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.