952799 - GeckoAppShell.getProxyForURI can be expensive during a pageload

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: Profile of mozilla.org

Using Traceview in Monitor, I see GeckoAppShell.getProxyForURI show up fairly high in the profile during a pageload.

For cnn.com (a desktop page): #19 at 21% taking over 500ms, being called 103 times
For mozilla.org (mobile page): #35 at 4% taking over 140ms, being called 36 times

Most of the time is taken in the URI parsing that happens here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java#2669

I would guess that most people don't have a proxy, but they are still paying the cost for the check. Setting "network.proxy.type" = 0 makes this go away, since we skip the system proxy check.

Most of the code for ProxySelector looks like it doesn't even need the Uri object. We already pass the scheme and host into getProxyForURI. We could add our own Java code to pull the proxy information from the Android system instead of using ProxySelector.

We could also add a proxy setting to our Setting UI and default to 0 (Direct). This would allow people who want proxies to set it up when they need it.

Comment 1

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

Attachment: Profile cnn.com

Comment 2

[Richard Newman [:rnewman]]

We already need to rework proxy handling to be Java-based: Bug 507641.

Comment 3

[Richard Newman [:rnewman]]

Also worth noting: every caller of this is providing spec/host/port etc. from an already parsed URI, so yes, parsing for validation is a waste of time.

The only thing we have to worry about is the disagreement between Java and Gecko on what's a valid URI -- I've seen some stack traces from attempted favicon lookups for google.com URIs that Gecko is fine with.

Let's try a trivial patch to memoize the proxy settings. That'll short-cut for free.

Comment 4

[Richard Newman [:rnewman]]

This is slightly harder to fix than it looks. The string-components interface is part of nsISystemProxySettings. The URI interface is part of the Java ProxySelect class.

I checked, and a stock KitKat device returns a non-null ProxySelector, so we can't even shortcut most fetches. (I still have a patch that reorders this work.)

The two ways forward here are:

* Don't use the system proxy selector. Write our own configuration system instead (complete with our own implementation of PAC handling). Even better if this transparently writes through to Gecko, too -- that way, most lookups can stay on the Gecko side (all but the stuff in Bug 507641), and we can eliminate nsAndroidSystemProxySettings.

* Implement a subclass of URI that is cheaper than `new URI(spec)`, and short-circuits its getHost() etc. calls to use the raw strings we already have. This will be cheaper for most proxy configurations, because we avoid actually parsing the URI string.

Comment 5

[Richard Newman [:rnewman]]

(In reply to Richard Newman [:rnewman] from comment #4)

> * Implement a subclass of URI that is cheaper than `new URI(spec)`

Alas, java.net.URI is final, so this isn't an option. *scraps that neat patch*

It also isn't clear that we should be using ProxySelector at all. See also

<https://github.com/shouldit/android-proxy-library>

But my conclusion seems to be: if you're not starting with a java.net.URI, and you care about perf, then the 'solution' is to not use the Android proxy settings.

Comment 6

[Richard Newman [:rnewman]]

Attachment: GeckoAppShell.getProxyForURI can be expensive during a pageload.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8350958 [details] [diff] [review]
GeckoAppShell.getProxyForURI can be expensive during a pageload.

Here's a part 0 that:

1. Reorders the lookup operations so that if there's no default proxy, we do no work. That might help on some devices.

2. Fixes some style warts.

Comment 8

[Richard Newman [:rnewman]]

Reading the code for ProxySelectImpl, the only parts of the URI that are used are the scheme and the host.

That implies that we can use the component constructor for URI, and avoid the parsing altogether. And if measurement suggests it, keep an LRU cache for proxy responses -- that CNN profile suggests that this was called 20 times, most of which will be for the same domain.

Comment 9

[Richard Newman [:rnewman]]

Attachment: GeckoAppShell.getProxyForURI can be expensive during a pageload. v2

This version doesn't bother parsing from the spec. It does not yet do any caching. Profile this, mfinkle?

Comment 10

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

Attachment: profile-cnn-proxy-patch1.png

Profile of loading cnn.com using the patch. getProxyForURI drops to #33, still a lot of time taken by Uri<init>, but this is better.

Comment 11

[Richard Newman [:rnewman]]

Attachment: GeckoAppShell.getProxyForURI can be expensive during a pageload. v3

This version adds a couple of LruCaches for URI creation. This is making a bet that looking up the hostname in a map, and doing a couple of string comparisons, is cheaper than creating a URI, given that loading something like CNN.com involves 89 requests, many of which will be to the same domain.

Please bench this!

Comment 12

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

Attachment: profile-cnn-proxy-patch2.png

Profile with patch v3: getProxyForURI moved down to #45

Comment 13

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

Attachment: profile-cnn-patch2-uri.png

Profile with patch v3, focus on Uri constructor. Not sure if I am reading it correctly, but it looks like getProxyForURI is called 101 times, while Uri.<init> is called 27 times. I take this to show the cache is having some affect.

Comment 14

[Richard Newman [:rnewman]]

Yup, that's the right reading.

We've knocked 90% off the real time (from the first profile to the last). That sounds pretty good to me, assuming we're happy with the constraints of this approach -- proxies don't get to see the whole URI, and we're caching up to 20 (number is tweakable) URI objects in memory at a time.

Comment 15

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

Comment on attachment 8351054 [details] [diff] [review]
GeckoAppShell.getProxyForURI can be expensive during a pageload. v3

Seems like a good improvement. I still wonder if pulling in a fork of ProxySelectorImpl would be worthwhile. Maybe in the future.

Comment 16

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/9bfd0ab40e7e

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9bfd0ab40e7e