[e10s] Improve perceived session restore duration

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Session Restore
P2
major
VERIFIED FIXED
3 years ago
5 months ago

People

(Reporter: ttaubert, Assigned: mconley)

Tracking

(Blocks: 3 bugs, {perf, topperf})

unspecified
Firefox 55
perf, topperf
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox55 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

Restoring a session with e10s seems to take a little more time currently as described in bug 1037179. The perception of that process plays quite a big role esp. with restore_on_demand=true. We can do better here:

1) The history of selected tabs should be restored first. We'll send messages to the docShells of all tabs we restore and so the more important and visible ones can react first and start loading their pages.

2) We send a message to restore the history and only after that is done the parent receives a message and sets tab label and icon. We need to do this afterwards because restoring the history resets label and icon. We however should also do this before sending the "restore history" message, that helps improving the whole UX a lot with my session as the response can take ~1s even on my machine.
Created attachment 8519529 [details] [diff] [review]
0001-Bug-1096013-e10s-Improve-perceived-session-restore-d.patch
Attachment #8519529 - Flags: review?(smacleod)

Comment 2

3 years ago
Thanks for filing this but, it summarizes my current grieve pretty well. I just want to add my impressions with regard to e10s:

When restoring a session with e10s enabled, I have to wait till all tab spinners have finished which takes up to 3 times longer here than without e10s. I have to wait since the ui is completely unresponsive during this procedure which wasn't the case without e10s.

The same problem occurs when loading a webpage. I have to wait till content is completely loaded because I can't do anything before it has finished loading. Without e10s I sometimes already closed a tab before it has finished loading because I could get a good impression of the content before loading finished. Especially the ability to start scrolling almost immediately after sending the page load request helped with that. With e10s this became as good as impossible.

I'm on a 2008 Macbook with core2duo @ 2 Ghz and 4GB of RAM.
Comment on attachment 8519529 [details] [diff] [review]
0001-Bug-1096013-e10s-Improve-perceived-session-restore-d.patch

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

Looks fine to me, just the one nit.

::: browser/components/sessionstore/SessionStore.jsm
@@ +1846,5 @@
>        this.saveStateDelayed();
>      }
>    },
>  
> +  updateTabLabelAndIcon(tab) {

It seems strange to me that we're just harvesting the tabdata off of browser, instead of passing it in. Maybe it's the name "update" that makes it feel wrong to me?

Would something like "refreshTabLabelAndIcon(tab)" make more sense? I'm being super picky though so "update" is fine if you prefer.
Attachment #8519529 - Flags: review?(smacleod) → review+
(In reply to beingalink from comment #2)
> When restoring a session with e10s enabled, I have to wait till all tab
> spinners have finished which takes up to 3 times longer here than without
> e10s. I have to wait since the ui is completely unresponsive during this
> procedure which wasn't the case without e10s.

That sounds like bug 1037179. The issue about the browser being unresponsive while loading resources or waiting on a network connection might have been filed somewhere. I definitely see this on my machine as well. We will probably have to improve the event handling here.
(In reply to Steven MacLeod [:smacleod] from comment #3)
> > +  updateTabLabelAndIcon(tab) {
> 
> It seems strange to me that we're just harvesting the tabdata off of
> browser, instead of passing it in. Maybe it's the name "update" that makes
> it feel wrong to me?
> 
> Would something like "refreshTabLabelAndIcon(tab)" make more sense? I'm
> being super picky though so "update" is fine if you prefer.

I actually think the "update" part is fine but I agree that we should pass |tabData| as an argument. That is a little more expressive than before.
https://hg.mozilla.org/integration/fx-team/rev/126811256577
Blocks: 1063169
tracking-e10s: --- → +
https://hg.mozilla.org/mozilla-central/rev/126811256577
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Depends on: 1097697
Backed out due to bug 1097697:

https://hg.mozilla.org/integration/fx-team/rev/fc4f585ae3a1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: ttaubert → nobody
Status: REOPENED → NEW
Assignee: nobody → mconley
Priority: -- → P2
(Assignee)

Comment 9

a year ago
Created attachment 8757973 [details]
Bug 1096013 - Show tab icons and titles as soon as possible instead of waiting for history restoration to complete.

Review commit: https://reviewboard.mozilla.org/r/56314/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56314/
Blocks: 1227630
Blocks: 1330635
Did you mean to reland this?
Flags: needinfo?(mconley)
(Assignee)

Comment 11

5 months ago
I seem to recall there being some test issues with this, which is why it's still WIP. I'll revisit this once I get bug 1256472 into the tree.
Flags: needinfo?(mconley)
(Assignee)

Updated

5 months ago
See Also: → bug 1256472
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

5 months ago
mozreview-review
Comment on attachment 8850351 [details]
Bug 1096013 - Give selected tabs highest priority during restoration.

https://reviewboard.mozilla.org/r/122974/#review125842

::: browser/components/sessionstore/SessionStore.jsm:3482
(Diff revision 1)
>      }
>  
> +    // If we restore the selected tab, make sure it goes first.
> +    let selectedIndex = aTabs.indexOf(tabbrowser.selectedTab);
> +    if (selectedIndex > -1) {
> +      this.restoreTab(tabbrowser.selectedTab, aTabData[selectedIndex]);

Out of curiosity; does this mean that the promise/ content task gets scheduled first, meaning that it'll be _most likely_ that this gets to go first, or is there some mechanism in place that ensures this?

I'm assuming that you're counting on the intrinsics of the messageManager to process messages in FIFO order?
Attachment #8850351 - Flags: review?(mdeboer) → review+

Comment 15

5 months ago
mozreview-review
Comment on attachment 8757973 [details]
Bug 1096013 - Show tab icons and titles as soon as possible instead of waiting for history restoration to complete.

https://reviewboard.mozilla.org/r/56314/#review125840

Looking good! You're making life too easy for our future GSoC student ;-)

I'd like to proof-read the JSDoc - I'll be sure to re-review before EOD today.

::: browser/components/sessionstore/SessionStore.jsm:2583
(Diff revision 2)
>  
>      // Neither a tab nor a window was found, return undefined and let the caller decide what to do about it.
>      return undefined;
>    },
>  
> +  updateTabLabelAndIcon(tab) {

Can you add a second argument here:
```js
updateTabLabelAndIcon(tab, tabData = null) {
  if (!tabData) {
    // ...
  }
  // Still no tabData? Throw.
},
```

The `activePageData` fetch is cheap enough to keep it inline here.

Also please add a JSDoc comment above.
Attachment #8757973 - Flags: review?(mdeboer) → review-
(Assignee)

Comment 16

5 months ago
mozreview-review-reply
Comment on attachment 8850351 [details]
Bug 1096013 - Give selected tabs highest priority during restoration.

https://reviewboard.mozilla.org/r/122974/#review125842

> Out of curiosity; does this mean that the promise/ content task gets scheduled first, meaning that it'll be _most likely_ that this gets to go first, or is there some mechanism in place that ensures this?
> 
> I'm assuming that you're counting on the intrinsics of the messageManager to process messages in FIFO order?

Yeah, with some very rare exceptions (sending a CPOW message from one process in response to a CPOW message from another process) messages are delivered and received in-order.

Anytime you're using the message manager, you get those in-order guarantees for those messages. Only when you have the freaky CPOW things do you have the potential of messages jumping the queue (at least, the last time I checked).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

5 months ago
mozreview-review
Comment on attachment 8757973 [details]
Bug 1096013 - Show tab icons and titles as soon as possible instead of waiting for history restoration to complete.

https://reviewboard.mozilla.org/r/56314/#review125922

Sweet! Thanks!
Attachment #8757973 - Flags: review?(mdeboer) → review+

Comment 21

5 months ago
mozreview-review
Comment on attachment 8850959 [details]
Bug 1096013 - Add a test that ensures that we don't lose the favicon for background tabs that crash.

https://reviewboard.mozilla.org/r/123470/#review125926

Again, sweet! ;-)
Attachment #8850959 - Flags: review?(mdeboer) → review+

Comment 22

5 months ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/21a926a8dffc
Show tab icons and titles as soon as possible instead of waiting for history restoration to complete. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/eb6768c39156
Give selected tabs highest priority during restoration. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/da1e8bddd9ba
Add a test that ensures that we don't lose the favicon for background tabs that crash. r=mikedeboer
Backed out for eslint failure at browser/components/sessionstore/SessionStore.jsm:2600:11:

https://hg.mozilla.org/integration/autoland/rev/9fc87b160c2b7a1751568e2882a022c3811a8488
https://hg.mozilla.org/integration/autoland/rev/95dc507a87ff624e78cd7a4980d107b34be625b1
https://hg.mozilla.org/integration/autoland/rev/45fa112a25d2954bb038d626214cc520e73cfe9a

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=da1e8bddd9ba7850d6bb391d42bb9a94f615bbdd&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=86261265&repo=autoland

> TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/sessionstore/SessionStore.jsm:2600:11 | 'tabData' is already declared in the upper scope. (no-shadow)
Flags: needinfo?(mconley)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 27

5 months ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/28229b5ba95a
Show tab icons and titles as soon as possible instead of waiting for history restoration to complete. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/dc7d1c21c857
Give selected tabs highest priority during restoration. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/24f52ebe6dc9
Add a test that ensures that we don't lose the favicon for background tabs that crash. r=mikedeboer
Backed out in https://hg.mozilla.org/integration/autoland/rev/24b5d95457e90c7859e08817c11769a0de9c0ed7 - it's sort of hard to tell since we're such silly buggers about pretending it makes sense to skip browser-chrome chunks when the contents of every single browser-chrome chunk completely change on every push which adds or removes a single browser-chrome test, but I sort of think that bug 1350482 wasn't so much "intermittent" as "permaorange on non-e10s".
Duplicate of this bug: 1350482
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 33

5 months ago
D'oh. Test should be e10s-only. Sorry about that. :/
Flags: needinfo?(mconley)

Comment 34

5 months ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a95b45f37b13
Show tab icons and titles as soon as possible instead of waiting for history restoration to complete. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/931854986f75
Give selected tabs highest priority during restoration. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/32f2f1e79d9f
Add a test that ensures that we don't lose the favicon for background tabs that crash. r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/a95b45f37b13
https://hg.mozilla.org/mozilla-central/rev/931854986f75
https://hg.mozilla.org/mozilla-central/rev/32f2f1e79d9f
Status: NEW → RESOLVED
Last Resolved: 3 years ago5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: Firefox 36 → Firefox 55
@ Mike Conley (:mconley) - Awesome job! As tab hoarder thank you very much!
Severity: normal → major
Status: RESOLVED → VERIFIED
Has Regression Range: --- → irrelevant
Has STR: --- → yes
status-firefox55: fixed → verified
Keywords: perf, topperf
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.