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.

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

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.

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.

(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 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.

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.

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

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.

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!

Profile with patch v3: getProxyForURI moved down to #45

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.

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 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.