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)
Firefox
Session Restore
Tracking
()
RESOLVED
FIXED
Firefox 29
People
(Reporter: smacleod, Assigned: smacleod)
Details
Attachments
(1 file, 2 obsolete files)
5.86 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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 2•10 years ago
|
||
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 3•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8348914 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/308fea805085
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 8•10 years ago
|
||
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.
Description
•