Closed
Bug 952799
Opened 11 years ago
Closed 11 years ago
GeckoAppShell.getProxyForURI can be expensive during a pageload
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 29
People
(Reporter: mfinkle, Assigned: rnewman)
References
Details
(Keywords: perf)
Attachments
(6 files, 2 obsolete files)
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.
Reporter | ||
Comment 1•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 2•11 years ago
|
||
We already need to rework proxy handling to be Java-based: Bug 507641.
Depends on: 507641
Assignee | ||
Comment 3•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
(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.
Assignee | ||
Comment 6•11 years ago
|
||
Assignee | ||
Comment 7•11 years ago
|
||
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.
Attachment #8350958 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 8•11 years ago
|
||
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.
Assignee | ||
Comment 9•11 years ago
|
||
This version doesn't bother parsing from the spec. It does not yet do any caching. Profile this, mfinkle?
Attachment #8351031 -
Flags: review?(mark.finkle)
Assignee | ||
Updated•11 years ago
|
Attachment #8350958 -
Attachment is obsolete: true
Attachment #8350958 -
Flags: review?(mark.finkle)
Reporter | ||
Comment 10•11 years ago
|
||
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.
Assignee | ||
Comment 11•11 years ago
|
||
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!
Attachment #8351054 -
Flags: review?(mark.finkle)
Assignee | ||
Updated•11 years ago
|
Attachment #8351031 -
Attachment is obsolete: true
Attachment #8351031 -
Flags: review?(mark.finkle)
Reporter | ||
Comment 12•11 years ago
|
||
Profile with patch v3: getProxyForURI moved down to #45
Reporter | ||
Comment 13•11 years ago
|
||
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.
Assignee | ||
Comment 14•11 years ago
|
||
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.
Reporter | ||
Comment 15•11 years ago
|
||
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.
Attachment #8351054 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 16•11 years ago
|
||
Target Milestone: --- → Firefox 29
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•