protocol handler caching is not so good

RESOLVED FIXED in Firefox 46

Status

()

Core
Networking
RESOLVED FIXED
8 years ago
a year ago

People

(Reporter: Dolske, Assigned: mcmanus)

Tracking

({perf})

Trunk
mozilla46
Points:
---
Bug Flags:
blocking1.9.2 -
wanted1.9.2 +

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
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.
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?
Flags: blocking1.9.2?
(Reporter)

Comment 2

8 years ago
...and shaver suggested just having nsIIOService.httpProtocol, .httpsProtocol, etc. Would help for the cases where we know what we want.
(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?
jar is not in necko. don't add a jar entry here.
Yeah, I'd take a patch, if everyone here is done their blockers.
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
(Reporter)

Comment 6

8 years ago
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?
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)
How do you propose applications handle other frequent protocols?

Comment 9

8 years ago
Do you get consecutive misses for the same protocol or are they random?
(Assignee)

Comment 10

a year ago
checking on this today, the list has been updated somewhere along the way to include data and https too.
(Assignee)

Comment 11

a year ago
(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
(Assignee)

Comment 12

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f17c4eac7f57
(Assignee)

Comment 13

a year ago
spot checking my cache misses the only obvious thing to add, at least as handled by necko, is about (and safe about)
(Assignee)

Comment 14

a year ago
Created attachment 8699520 [details] [diff] [review]
cache about protocol handlers
Attachment #8699520 - Flags: review?(honzab.moz)
(Assignee)

Updated

a year ago
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Attachment #8699520 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 15

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2a07b55949a1e24069ca4695af932ea710067f8
Bug 524232 - cache about: protocol handlers r=mayhemer

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a2a07b55949a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.