Closed Bug 945809 Opened 11 years ago Closed 10 years ago

[Session Restore] Measure Longest Data Collection Operations in Content Script

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: smacleod, Assigned: smacleod)

Details

Attachments

(1 file, 2 obsolete files)

Data collection has been moved to be broadcast on changes from the frame script. We need to check the current coverage of longest operation Telemetry and see if we can compare that to the new broadcasting efforts.

We should add Telemetry measurements to the data collection routines to record the longest blocking operation. We can probably do this by wrapping the lazy collection closures. We'll also need these measurements to check progress with sending smaller differential updates from the frame script in the future.

Also, we might need to disable the collection of these telemetry measurements in multiprocess Firefox for now.
This adds two new Telemetry measurements to the content script:
1) A timing of how long collection takes inside the lazy closure.
2) A timing of the entire collection process before sending a message out of the content script.

For (1) should we maybe be taking distinct measurements for each "key" being collected, so we could decide where to focus our efforts in splitting up and optimizing collection?

The data from (2) should give us a better idea of how long we're blocking collecting data before sending it.

Apparently telemetry data from child processes will just be dropped, so we should be okay to just leave these measurements without a check for remote tabs.
Attachment #8345008 - Flags: feedback?(ttaubert)
Attachment #8345008 - Flags: feedback?(dteller)
Comment on attachment 8345008 [details] [diff] [review]
Patch - Add telemetry to the content script

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

> Apparently telemetry data from child processes will just be dropped, so we should be okay to just leave these measurements without a check for remote tabs.

So you need to send it to the parent, is that it?

Also, you're not using FX_SESSION_RESTORE_CONTENT_COLLECT_DATA_MS yet, are you?

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +46,5 @@
>        cachedValue = fn();
>        cached = true;
>      }
>  
> +    TelemetryStopwatch.finish("FX_SESSION_RESTORE_CONTENT_COLLECT_DATA_MS");

I don't think that this probe is useful.

@@ +422,5 @@
>        data[key] = this._data.get(key)();
>      }
>  
> +    TelemetryStopwatch.finish(
> +      "FX_SESSION_RESTORE_CONTENT_COLLECT_DATA_LONGEST_OP_MS");

This one looks good, but I'd like a try/finally, just in case.
Attachment #8345008 - Flags: feedback?(dteller) → feedback+
Comment on attachment 8345008 [details] [diff] [review]
Patch - Add telemetry to the content script

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

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +46,5 @@
>        cachedValue = fn();
>        cached = true;
>      }
>  
> +    TelemetryStopwatch.finish("FX_SESSION_RESTORE_CONTENT_COLLECT_DATA_MS");

After thinking about this more I guess that David is right. We can always dig deeper should we see long collection times but in that case we would need to find out what's taking so long anyway.

@@ +422,5 @@
>        data[key] = this._data.get(key)();
>      }
>  
> +    TelemetryStopwatch.finish(
> +      "FX_SESSION_RESTORE_CONTENT_COLLECT_DATA_LONGEST_OP_MS");

Please don't let us wrap everything in try/catch again. That's really an anti-pattern. We have bigger problems than a failing telemetry probe should something throw in here.
Attachment #8345008 - Flags: feedback?(ttaubert) → feedback+
The patch now transmits telemetry data out of the content scripts, and only contains a single histogram.
Attachment #8345008 - Attachment is obsolete: true
Attachment #8348914 - Flags: review?(ttaubert)
Comment on attachment 8348914 [details] [diff] [review]
Patch - Add telemetry to the content script v2

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +628,5 @@
> +   */
> +  recordTelemetry: function (telemetry) {
> +    for (let histogramId in telemetry){
> +      Telemetry.getHistogramById(histogramId).add(telemetry[histogramId]);
> +    }

Would be good to merge this with the code in SessionFile in a follow-up.
Attachment #8348914 - Flags: review?(ttaubert) → review+
Attachment #8348914 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/308fea805085
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: