Closed Bug 1228020 Opened 4 years ago Closed 4 years ago

[e10s] Review FX_PAGE_LOAD_MS in e10s

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
e10s + ---
firefox46 --- fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
I'm not entirely sure about this probe, as it seems to be handled entirely in the parent process. I haven't found yet how/if the parent process knows that the child process has finished loading.
Instrumenting the probe, it seems ok to me. If a page takes ~5s to load (based on the spinner), it receives a measure of ~5s. I believe that this is what we want.

Minor issues: this probe is used to measure *all* pages, including about:blank and other about: pages, which is probably driving the costs down. This looks pretty trivial to fix.

What do you think, Vladan?
Flags: needinfo?(vladan.bugzilla)
* If it's trivial to filter out about: pages from this probe, sure go for it. Make sure you check whether this made any difference in the distributions or not.
* So what does FX_PAGE_LOAD_MS "mean"? Is it really just reporting on spinner duration? Does it record samples for sub-resources and dynamic fetches (e.g. scrolling down my Facebook newsfeed)? And does it measure different things on e10s & non-e10s?
Flags: needinfo?(vladan.bugzilla) → needinfo?(dteller)
This probe waits for the entire page to be loaded from the network, including static content (I have tested with both iframes, images and scripts, but I haven't tested movies or audio). It is not updated for dynamic fetches.

If I read the source code correctly, the e10s version might be a little more noisy, as the stopwatch is in the parent process while the loading itself is in the content process, so there's an IPC call both before the `start` and before the `finish`. I haven't attempted to trace the actual IPC call. Other than that, my experiments haven't shown any difference between e10s and non-e10s.
Flags: needinfo?(dteller)
Do you need further details?
Flags: needinfo?(vladan.bugzilla)
No, this seems good enough. Filter out the about: page measurements if it's trivial to do so

Chris: update your "telemetry debugging" extension to (optionally) change the tab title to include to reflect the loading time (equivalent to FX_PAGE_LOAD_MS). You can probably listen for the same events as the probe code
Flags: needinfo?(vladan.bugzilla) → needinfo?(chutten)
I'm confused about what a ms count in the page title would provide that the absence of the spinner wouldn't. And how a user would use this information to decide if something is wrong.

With BHR and user-interaction-active, it is clear: if you get an indication that there's a hang being reported and you didn't feel one, then that's a bug. Ditto the reverse. If you are being active and you see an inactive blue, that's a bug. Ditto being inactive and seeing an active red.
Flags: needinfo?(chutten) → needinfo?(vladan.bugzilla)
> I'm confused about what a ms count in the page title would provide that the absence of the spinner wouldn't.

i) Yoric speculates that there are different "kinds of spins": a spin for initial main page loading, and spinners for everything else. He believes FX_PAGE_LOAD_MS instruments the first kind of spin.

ii) Yoric and I also discussed your idea that having 2 event threads in e10s would cause e10s to log smaller values in FX_PAGE_LOAD_MS. Yoric is of the opinion that this indeed will happen but that it is also a reflection of a real improvement.

Yoric: can you verify that i) above is true
Chris: it seems there's enough ambiguity around what this probe measures and how it will be different on e10s to justify extending your extension as per comment 6

> And how a user would use this information to decide if something is wrong.

With this functionality added, we can do quick gut-checks on the reported page-laod time measurements. e.g. if measurement reports 1 second, but spinner never stopped spinning, I would assume something was wrong
Flags: needinfo?(vladan.bugzilla)
(In reply to Vladan Djeric (:vladan) -- please needinfo | PTO Dec 14-18. Email or IRC if urgent from comment #8)
> > I'm confused about what a ms count in the page title would provide that the absence of the spinner wouldn't.
> 
> i) Yoric speculates that there are different "kinds of spins": a spin for
> initial main page loading, and spinners for everything else. He believes
> FX_PAGE_LOAD_MS instruments the first kind of spin.

Yep, confirmed. FX_PAGE_LOAD_MS is only updated for toplevel requests, while the spinner is updated for all requests. If I read the latter's code correctly, it is removed once we have finished the first network request of the document (and restarted for any later network request of the document), which looks highly suspicious to me, but may be good UX-wise.
Bug 1228020 - Removing about: pages from FX_PAGE_LOAD;r?dao

about: pages (including about:blank) are loaded quite often and do not
reflect any useful real-world performance for this histogram, so we
remove them.

Also, documenting a tad more the histogram.
Attachment #8698424 - Flags: review?(dao)
Comment on attachment 8698424 [details]
MozReview Request: Bug 1228020 - Removing about: pages from FX_PAGE_LOAD;r=dao

Looks like you removed the blank line at the end of browser.js. Please don't do that.
Attachment #8698424 - Flags: review?(dao) → review+
tracking-e10s: --- → +
Attachment #8698424 - Attachment is obsolete: true
Attachment #8698915 - Flags: review?(dteller) → review+
Hi, this failed to apply:

applying 92507884fb1e
patching file browser/base/content/browser.js
Hunk #2 FAILED at 8311
1 out of 2 hunks FAILED -- saving rejects to file browser/base/content/browser.js.rej
patch failed to apply
abort: fix up the merge and run hg transplant --continue
Flags: needinfo?(dteller)
Comment on attachment 8698424 [details]
MozReview Request: Bug 1228020 - Removing about: pages from FX_PAGE_LOAD;r=dao

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27971/diff/2-3/
Attachment #8698424 - Attachment description: MozReview Request: Bug 1228020 - Removing about: pages from FX_PAGE_LOAD;r?dao → MozReview Request: Bug 1228020 - Removing about: pages from FX_PAGE_LOAD;r=dao
Attachment #8698424 - Attachment is obsolete: false
Attachment #8698424 - Flags: review+ → review?(dteller)
Flags: needinfo?(dteller)
Keywords: checkin-needed
Attachment #8698424 - Attachment is obsolete: true
Attachment #8698424 - Flags: review?(dteller)
https://hg.mozilla.org/mozilla-central/rev/e2ac14b932c0
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Depends on: 1364957
You need to log in before you can comment on or make changes to this bug.