Closed Bug 874533 Opened 7 years ago Closed 2 years ago

Use speculative connect for pending tabs in the restore queue

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: ttaubert, Assigned: beekill)

References

(Blocks 2 open bugs)

Details

(Keywords: perf, Whiteboard: [has draft patch])

Attachments

(5 files, 1 obsolete file)

Nick and his team came up with the idea of speeding up the process of restoring a session by speculatively connecting to hosts of tabs that are currently pending to be restored and that will be restored automatically (not on demand).
Nick, I know that the final patch will not use nsISpeculativeConnect but a new module with a couple of heuristics - nevertheless I spent some time on the weekend to implement what we talked about.

I think this basically should do the trick and all left to do is switch to the new API when it's ready. Please feel free to take over the patch and test or adjust it to your needs!
Keywords: perf
Whiteboard: [has draft patch]
Hi Tim! I'm curious, which Nick did you mean and what new module are you talking about? Would using speculative connect here be reasonable still? (My guess: yes)
Blocks: ss-perf
Flags: needinfo?(ttaubert)
Probably still reasonable. I'm pretty sure I meant Nick Hurley, who is CC'ed. No idea what module I was talking about... is there a spec connect JSM?
Flags: needinfo?(ttaubert)
(In reply to Tim Taubert [:ttaubert] from comment #3)
> No idea what module I was talking about... is there a spec connect
> JSM?

No, just nsISpeculativeConnect, which is simple enough to use as-is...
I suspect the module Tim was talking about is nsINetworkPredictor, not a JSM, but what was being worked on when this bug was filed. This particular bit of functionality never made it into a production version of that service, and after now multiple years of experience with the predictor, I'm not certain that's actually the right place.

I think the patch that Tim put up "back in the day" is probably the right way to do this - we don't need the "smarts" of the predictor to do this, so it seems silly to increase the amount of data the predictor is storing just to make this work.

Another thing that might be useful is to speculatively connect to the host for unloaded tabs when the user hovers their mouse over them (we might already do this, I don't know). We do the same thing for links on a page through the predictor (that way we can avoid doing it if the predictor is disabled), so we might want to do the same thing for this use case. For connecting to hosts we already know are just waiting to be loaded, though, nsISpeculativeConnect is fine.
Attached patch spec.v1.patch (obsolete) — Splinter Review
I implement this based on the patch by Tim and Nicholas's suggestion.
Attachment #8876165 - Flags: feedback?(mdeboer)
Comment on attachment 8876165 [details] [diff] [review]
spec.v1.patch

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

Still a few things to do here, but looks promising.

::: browser/components/sessionstore/SessionStore.jsm
@@ +960,4 @@
>            Services.obs.notifyObservers(browser, NOTIFY_TAB_RESTORED);
>          }
>  
> +        // Remove the mouse hover listener after the tab has loaded

nit: missing dot.

@@ +960,5 @@
>            Services.obs.notifyObservers(browser, NOTIFY_TAB_RESTORED);
>          }
>  
> +        // Remove the mouse hover listener after the tab has loaded
> +        if (tab.__connection_prepared) {

I don't think you need to do this at all, since the tab _will_ be hover once in the near future. And when it fires and the connection has already been made, it'll be a no-op.

@@ +3293,4 @@
>        let select = t == selectTab - 1;
>        let createLazyBrowser = restoreTabsLazily && !select && !tabData.pinned;
>  
> +      let activeIndex = (tabData.index || tabData.entries.length) - 1;

You don't need to move this code.

@@ +3326,4 @@
>            tabbrowser.removeTab(tabbrowser.tabs[0]);
>          }
>          tabsToRemove = 0;
> +        // Prepare connection to the host when users hover mouse over this

nit: You need to align this comment properly with the closing curly brace below.

@@ +3329,5 @@
> +        // Prepare connection to the host when users hover mouse over this
> +        // tab. If we're not restoring on demand, we'll prepare connection
> +        // when we're restoring next tab.
> +      } else if ((!tabData.pinned && this._prefBranch.getBoolPref("sessionstore.restore_on_demand")) ||
> +                 (tabData.pinned && this._prefBranch.getBoolPref("sessionstore.restore_pinned_tabs_on_demand"))) {

Why the two clauses? It seems to me like you can leave `tabData.pinned` out of it.
Getting a pref value is not free, so you need to 'cache' it before its first use at `let restoreTabsLazily`.
Then you can make this simply `if (restoreOnDemand) {`

@@ +3330,5 @@
> +        // tab. If we're not restoring on demand, we'll prepare connection
> +        // when we're restoring next tab.
> +      } else if ((!tabData.pinned && this._prefBranch.getBoolPref("sessionstore.restore_on_demand")) ||
> +                 (tabData.pinned && this._prefBranch.getBoolPref("sessionstore.restore_pinned_tabs_on_demand"))) {
> +          tab.__onMouseOver = () => {

Please make this look better by adding a method `speculativeConnectOnHover(tab, url)`, where you setup this listener and make the connection on tab hover.

@@ +3337,5 @@
> +              let url = tabData.entries[activeIndex].url;
> +              this.prepareConnectionToHost(url);
> +            }
> +          };
> +          tab.addEventListener("mouseover", tab.__onMouseOver);

In that function, please add `, {once: true}` to cleanup the listener properly.

@@ +3431,4 @@
>    },
>  
>    /**
> +   * Prepare connection to host beforehand

nit: missing dot.

@@ +3436,5 @@
> +   * @param url
> +   *        URL of the host
> +   */
> +  prepareConnectionToHost(url) {
> +    if (url) {

There's no need to guard like this; URL is always set, right?
What _would_ be useful here is to check if it's not an 'about:' URL and return if so.

@@ +3711,5 @@
>          this.restoreNextTab();
> +
> +        // Check if the current tab is currently queued and will be restored
> +        // after the currently loading tabs. If so, prepare the connection
> +        // to its current URL's host to speed up page loading.

nit: you're saying 'current' quite a bit here ;-)
Attachment #8876165 - Flags: feedback?(mdeboer) → feedback+
Attached patch spec.v2.patchSplinter Review
Changed as you requested.

Mike, how should I write a test for this?
Attachment #8876165 - Attachment is obsolete: true
Attachment #8878292 - Flags: feedback?(mdeboer)
(In reply to Bao Quan [:beekill] from comment #8)
> Mike, how should I write a test for this?

Not a clue. I think we'll just have to assume it works... the only thing I can think of is that you can test this by stubbing the `prepareConnectionToHost` method in the test to set a flag and in the test you'd assert that the flag was set when you simulate hovering the mouse.
Assignee: nobody → nnn_bikiu0707
Status: NEW → ASSIGNED
Comment on attachment 8878292 [details] [diff] [review]
spec.v2.patch

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

Yeah, this looks good to me, I think it'd be ready for reviews in this state.
Attachment #8878292 - Flags: feedback?(mdeboer) → feedback+
What happened to the approach with the mock 'nsISpeculativeConnect' and/ or 'nsIIOService2' service? `Services.io` is a lazy-loaded instance of the 'nsIIOService2' service, which you probably can mock if you do it before doing anything else in that test.
Because you need to write too much boilerplate code to be able to test it and that's not maintainable.
Flags: needinfo?(nnn_bikiu0707)
Attachment #8880683 - Flags: review?(mdeboer)
Attachment #8880684 - Flags: review?(mdeboer)
Attached file browser_874533.js
I misunderstood you. Stupid me!

I tried to mock the nsISpeculativeConnect, that required me to mock Services.io as well. However, I mocked both of them but it crashed the browser. The browser crashed when nsIClassInfo is queried from Services.io - probably from C++ code.

I attached the test file so you can check it out.

After a search in searchfox, I found out some places use speculative connect like bug 1006103, bug 789889 and bug 790882. These bugs don't have tests for me to follow. So I guess they just assumed it worked.
Flags: needinfo?(nnn_bikiu0707) → needinfo?(mdeboer)
I'll take a look at this test today.
You know what, now that I've taken a good look at how the sessionstore.debug pref is used here, I think it's a nice find and is probably the cheapest way of testing this at the moment. Please continue with the test as-is.
Again, apology for the delay, but that was inevitable.

The code has bitrotted a bit, so please rebase your patches on top of m-c tip. Thanks!
Flags: needinfo?(mdeboer)
Comment on attachment 8880683 [details]
Bug 874533 - Part 1: Speculative connect when users hover mouse on a tab or when restoring tabs automatically.

https://reviewboard.mozilla.org/r/152018/#review162810
Attachment #8880683 - Flags: review?(mdeboer) → review+
Comment on attachment 8880684 [details]
Bug 874533 - Part 2: Test connections prepared when users hover mouse on tabs or when restoring automatically.

https://reviewboard.mozilla.org/r/152020/#review162812

r=me with the test and the test asset renamed to be more descriptive, instead of just the bug number.
Attachment #8880684 - Flags: review?(mdeboer) → review+
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/36fe0b0b3b4c
Part 1: Speculative connect when users hover mouse on a tab or when restoring tabs automatically. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/b194f4fb6731
Part 2: Test connections prepared when users hover mouse on tabs or when restoring automatically. r=mikedeboer
Duplicate of this bug: 795303
Backed out for eslint failures, e.g. in browser/components/sessionstore/SessionStore.jsm:3437:

https://hg.mozilla.org/integration/autoland/rev/6542a177cd648b30394821f1055e548af7b3ae1a
https://hg.mozilla.org/integration/autoland/rev/462ef83227d51df19bf541532cfc1f457a097703

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b194f4fb673102e0606b347a4bc19a201657173f&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=115290847&repo=autoland

TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/sessionstore/SessionStore.jsm:3437:17 | newURI's last parameters are optional. (mozilla/no-useless-parameters)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/sessionstore/test/browser_speculative_connect.html:8:8 | Newline required at end of file but not found. (eol-last)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/sessionstore/test/browser_speculative_connect.js:15:2 | Unnecessary semicolon. (no-extra-semi)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/sessionstore/test/browser_speculative_connect.js:38:26 | Strings must use doublequote. (quotes)
Flags: needinfo?(nnn_bikiu0707)
Flags: needinfo?(nnn_bikiu0707)
Attachment #8880683 - Flags: review+ → review?(mdeboer)
Attachment #8880684 - Flags: review+ → review?(mdeboer)
Attachment #8880683 - Flags: review?(mdeboer)
Fixed eslint error. Mike, can you land this for me? Thanks!
Flags: needinfo?(mdeboer)
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6d0bcaaf7695
Part 1: Speculative connect when users hover mouse on a tab or when restoring tabs automatically. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/b9710c3f87d5
Part 2: Test connections prepared when users hover mouse on tabs or when restoring automatically. r=mikedeboer
Flags: needinfo?(mdeboer)
https://hg.mozilla.org/mozilla-central/rev/6d0bcaaf7695
https://hg.mozilla.org/mozilla-central/rev/b9710c3f87d5
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Depends on: 1383073
Depends on: 1418009
No longer depends on: 1418009
You need to log in before you can comment on or make changes to this bug.