Last Comment Bug 524232 - protocol handler caching is not so good
: protocol handler caching is not so good
Status: RESOLVED FIXED
: perf
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla46
Assigned To: Patrick McManus [:mcmanus]
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-23 16:44 PDT by Justin Dolske [:Dolske]
Modified: 2016-01-05 16:05 PST (History)
6 users (show)
mbeltzner: blocking1.9.2-
mbeltzner: wanted1.9.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
cache about protocol handlers (3.98 KB, patch)
2015-12-17 09:35 PST, Patrick McManus [:mcmanus]
honzab.moz: review+
Details | Diff | Splinter Review

Description Justin Dolske [:Dolske] 2009-10-23 16:44:26 PDT
While looking at nsIOService::GetProtocolHandler(), I noticed it has support for caching a previously-obtained handler. Otherwise it does a pref lookup, and then a getService for the requested protocol.

The cache is super simplistic, though. It's limited to exactly these entries:

70 static const char gScheme[][sizeof("resource")] =
71     {"chrome", "file", "http", "jar", "resource"};

I logged cache misses while doing a bit of browsing, and get a flood for "https", "moz-anno" and to a lesser degree "javascript", "data". During startup I get a dozen or two misses for "about" and "moz-safe-about".

Seems like we should either expand the list of cached schemes, or make this into a dynamically-expandable cache. This would probably be a nice little perf boost for mobile, saving 3 XPCOM calls per miss.
Comment 1 Shawn Wilsher :sdwilsh 2009-10-23 16:54:59 PDT
Requesting blocking to get wanted+.  This won't be hard to at least add some of the common ones to the static cache, and maybe a follow-up for the dynamic one?
Comment 2 Justin Dolske [:Dolske] 2009-10-23 16:56:19 PDT
...and shaver suggested just having nsIIOService.httpProtocol, .httpsProtocol, etc. Would help for the cases where we know what we want.
Comment 3 Shawn Wilsher :sdwilsh 2009-10-23 17:15:49 PDT
(In reply to comment #2)
> ...and shaver suggested just having nsIIOService.httpProtocol, .httpsProtocol,
> etc. Would help for the cases where we know what we want.
While we don't have many jar files nowadays, it might be a good idea to have a property for the jar protocol too?
Comment 4 Christian :Biesinger (don't email me, ping me on IRC) 2009-10-24 01:17:06 PDT
jar is not in necko. don't add a jar entry here.
Comment 5 Mike Beltzner [:beltzner, not reading bugmail] 2009-10-29 11:44:22 PDT
Yeah, I'd take a patch, if everyone here is done their blockers.
Comment 6 Justin Dolske [:Dolske] 2009-10-29 13:15:52 PDT
I did the trivial patch earlier, but the tricky part is that the cache will only hold things that support nsIWeakReference, which some of those other protocol handlers don't.

Is fixing that as simple as just adding the relevant interface to the NS_IMPL_ISUPPORTS#() list?
Comment 7 Christian :Biesinger (don't email me, ping me on IRC) 2009-10-30 07:33:37 PDT
https://developer.mozilla.org/en/nsSupportsWeakReference

But I'd rather you didn't add cache entries for protocol handlers that aren't implemented in necko (like the moz-anno one or some others mentioned in comment 0)
Comment 8 Shawn Wilsher :sdwilsh 2009-10-30 09:30:46 PDT
How do you propose applications handle other frequent protocols?
Comment 9 neil@parkwaycc.co.uk 2009-12-05 07:49:49 PST
Do you get consecutive misses for the same protocol or are they random?
Comment 10 Patrick McManus [:mcmanus] 2015-12-17 06:37:02 PST
checking on this today, the list has been updated somewhere along the way to include data and https too.
Comment 11 Patrick McManus [:mcmanus] 2015-12-17 06:43:27 PST
(In reply to Patrick McManus [:mcmanus] from comment #10)
> checking on this today, the list has been updated somewhere along the way to
> include data and https too.

https://bugzilla.mozilla.org/show_bug.cgi?id=1149228
Comment 12 Patrick McManus [:mcmanus] 2015-12-17 07:42:09 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f17c4eac7f57
Comment 13 Patrick McManus [:mcmanus] 2015-12-17 07:43:15 PST
spot checking my cache misses the only obvious thing to add, at least as handled by necko, is about (and safe about)
Comment 14 Patrick McManus [:mcmanus] 2015-12-17 09:35:43 PST
Created attachment 8699520 [details] [diff] [review]
cache about protocol handlers
Comment 15 Patrick McManus [:mcmanus] 2016-01-05 08:12:39 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2a07b55949a1e24069ca4695af932ea710067f8
Bug 524232 - cache about: protocol handlers r=mayhemer
Comment 16 Wes Kocher (:KWierso) 2016-01-05 16:05:20 PST
https://hg.mozilla.org/mozilla-central/rev/a2a07b55949a

Note You need to log in before you can comment on or make changes to this bug.