789889 - Speculatively open connections for links before dispatch a click event

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

We spend some time deciding if a user is actually clicking on a link rather than making a gesture, so it could be a pref win to start pre-loading a link while we're doing that deciding.

One issue I could see with this is using up more network data, but maybe we can start by just doing this on wifi. I know there's some other pre-loading work going on, so this may be a dupe, or similar to an existing bug.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Good idea. The necko code has a speculative connect API [1] that we should be able to use for this purpose.

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsISpeculativeConnect.idl

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

This works (verified using wireshark that it opens the connection). We need to figure out if we want to limit this for privacy/network-use/other concerns though.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Adding Patrick McManus for any comments he might have on this. Patrick, do you have any suggestions for how to test the impact of this change and/or heuristics that might be appropriate to tune the behaviour further?

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 659723 [details] [diff] [review]
Patch

I like the approach here. If Patrick has no reservations, we should move ahead.

Comment 5

[Patrick McManus [:mcmanus]]

this is great.

you could measure it by tracking time to first on data available for the channel with and without the change. Its possible that persistent connections may obscure the value of this for the median but the tail at least ought to improve.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch to measure time from click event to data available

I hacked in some code to print out the time between the click event is dispatched and the data becomes available. I tried this out on a test page with two links (one to a HTTPS href and one to a HTTP href) and clicked on the links 10 times each with the speculation code turned on and off. Some of the times had some sort of stalls (including the OCSP lookup for the first HTTPS connection) so I threw away everything > 1s and here are the results (times are in microseconds):

Without speculation (baseline numbers):
HTTPS: Samples: 7
       Average: 526401
       Stddev: 208192
       Max: 657562
       Min: 453979

HTTP:  Samples: 9
       Average: 256798
       Stddev: 86726.1
       Max: 286346
       Min: 234008

With speculation (23% improvement on HTTPS, 25% improvement on HTTP):

HTTPS: Samples: 10
       Average: 404077
       Stddev: 178990
       Max: 829040
       Min: 271972

HTTP:  Samples: 9
       Average: 190751
       Stddev: 70822.4
       Max: 245452
       Min: 160034

The above numbers are probably not super-reliable but definitely show a non-trivial improvement.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Btw, I'm waiting to get bug 761706 resolved first so that I can reliably send network connection updates to browser.js, and only do the speculative connections on wi-fi. If the extra network activity isn't a concern at all then I don't need to make any changes to this patch, I think.

Comment 8

[(dormant account)]

(In reply to Kartikaya Gupta (:kats) from comment #7)
> Btw, I'm waiting to get bug 761706 resolved first so that I can reliably
> send network connection updates to browser.js, and only do the speculative
> connections on wi-fi. If the extra network activity isn't a concern at all
> then I don't need to make any changes to this patch, I think.

I think we should do this on all connection types. Connection setup overhead is pretty tiny in terms of a hit on data traffic whereas the perf improvement is significant.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Taras Glek (:taras) from comment #8)
> (In reply to Kartikaya Gupta (:kats) from comment #7)
> > Btw, I'm waiting to get bug 761706 resolved first so that I can reliably
> > send network connection updates to browser.js, and only do the speculative
> > connections on wi-fi. If the extra network activity isn't a concern at all
> > then I don't need to make any changes to this patch, I think.
> 
> I think we should do this on all connection types. Connection setup overhead
> is pretty tiny in terms of a hit on data traffic whereas the perf
> improvement is significant.

I agree with Taras. Speculatively opening a connection shouldn't really affect data charges either, would it?

Comment 10

[Patrick McManus [:mcmanus]]

bandwidth doesn't concern me for the syn - its tiny. I might give a little thought to whether or not you were spinning up the radio when otherwise it would remain quiet (and therefore impact battery); but I suspect you're about to enter a period of activity anyhow so it's a good thing to get that warm up out of the way.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 659723 [details] [diff] [review]
Patch

Ok then, requesting review (same patch, I haven't thought of anything that might need to change).

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 659723 [details] [diff] [review]
Patch

Should we be handling relative URLs? Maybe speculativeConnect works OK from relative URLIs.

We jump through some hoops in NativeWindow.contextmenus to handle those:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1491

called from:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1514

Waiting to r+ based on relative URI answers

Comment 13

[Kartikaya Gupta (email:[e-mail])]

For HTML content the href property is already the absolute URL (and resolved against the base URI if there is one). The browser.js code you linked to calls makeURLAbsolute only if it's an xlink element. I specifically didn't support speculative connections on xlink elements because I have no idea where they might be used and imagine they're not common enough to need to support for now.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08dc8d488f5d

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08dc8d488f5d