Closed Bug 972797 Opened 6 years ago Closed 6 years ago

[Session Restore] Need more Telemetry on collection

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30

People

(Reporter: Yoric, Assigned: Yoric)

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

According to http://telemetry.mozilla.org/#nightly/29/FX_SESSION_RESTORE_COLLECT_DATA_MS, we still haven't finished making data collection jank-free. Additional Telemetry would be very useful to measure improvements.
Assignee: nobody → dteller
Attached patch Yet more telemetry (obsolete) — Splinter Review
Attachment #8377278 - Flags: review?(ttaubert)
Comment on attachment 8377278 [details] [diff] [review]
Yet more telemetry

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +2104,5 @@
>        session: session,
>        scratchpads: scratchpads,
>        global: this._globalState.getState()
>      };
> +    TelemetryStopwatch.stop("FX_SESSION_RESTORE_COLLECT_GLOBAL_STATE_DATA_MS");

GlobalState.getState() just returns an object, I don't think it's worth to measure that.

@@ +2113,2 @@
>        state.lastSessionState = LastSession.getState();
> +      TelemetryStopwatch.stop("FX_SESSION_RESTORE_COLLECT_LAST_SESSION_DATA_MS");

This also just returns an object, no need to measure that.

@@ +2149,2 @@
>      if (!this._isWindowLoaded(aWindow))
>        return;

We should start this after the _isWindowLoaded() check. If there's any other possibility to bail out in this method we should call TelemetryStopWatch.cancel().
Attachment #8377278 - Flags: review?(ttaubert) → feedback+
Attachment #8377278 - Attachment is obsolete: true
Attachment #8377536 - Flags: review?(ttaubert)
Attachment #8377536 - Flags: review?(ttaubert) → review+
Status: NEW → ASSIGNED
Backed out for:
https://tbpl.mozilla.org/php/getParsedLog.php?id=34848570&tree=Fx-Team

07:38:39     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_354894_perwindowpb.js | Only one browser window should be open initially (nsIWindowMediator)
07:38:39     INFO -  ************************************************************
07:38:39     INFO -  * Call to xpconnect wrapped JSObject produced this error:  *
07:38:39     INFO -  [Exception... "[JavaScript Error: "TelemetryStopwatch.stop is not a function" {file: "resource:///modules/sessionstore/SessionStore.jsm" line: 2168}]'[JavaScript Error: "TelemetryStopwatch.stop is not a function" {file: "resource:///modules/sessionstore/SessionStore.jsm" line: 2168}]' when calling method: [nsISessionStore::getBrowserState]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_354894_perwindowpb.js :: browserWindowsCount :: line 84"  data: yes]


remote:   https://hg.mozilla.org/integration/fx-team/rev/1a023bc837da
Ugh, it's .finish(). :(
Relanded with s/stop/finish:

https://hg.mozilla.org/integration/fx-team/rev/92375c1f07ef

And ran sessionstore tests locally before to make sure we don't break the tree again.
https://hg.mozilla.org/mozilla-central/rev/92375c1f07ef
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
(In reply to Tim Taubert [:ttaubert] from comment #6)
> Ugh, it's .finish(). :(

/me feels stupid
Thanks for picking this up.
Verified with latest build of Nightly
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.