Closed Bug 896382 Opened 6 years ago Closed 6 years ago

UserAgentOverrides.jsm should cache the override (or that there's no override) for each host to avoid doing the same iterative string matching for every HTTP request

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

No description provided.
Keywords: perf
Attached patch hostCache.diff (obsolete) — Splinter Review
Does this help?
Attachment #779167 - Flags: review?(doug.turner)
Comment on attachment 779167 [details] [diff] [review]
hostCache.diff

>+const MAX_OVERRIDE_FOR_HOST_CACHE_SIZE = 100;

It might make sense to let this grow a little bigger. 100 could be reached pretty quickly when loading a couple of sites that load resources from various different (sub) domains, which isn't an uncommon pattern.
Comment on attachment 779167 [details] [diff] [review]
hostCache.diff

Review of attachment 779167 [details] [diff] [review]:
-----------------------------------------------------------------

giving this review to Nick
Attachment #779167 - Flags: review?(doug.turner) → review?(hurley)
Comment on attachment 779167 [details] [diff] [review]
hostCache.diff

Review of attachment 779167 [details] [diff] [review]:
-----------------------------------------------------------------

So I am unable to reproduce this on any of my devices (desktop has getOverrideForURI taking 0% of CPU, and B2G device crashes loading the page while profiling on a regular build), so I can't say whether or not this patch improves Doug's use case. Doug I'm going to have to leave the verification up to you :) However, the idea does seem sane to me, so I'll r+ it.

Dao, I would, however, up the cache limit like you mentioned in comment 2. Somewhere between 200 and 500 sounds sane, to me. I'll leave the final value up to you
Attachment #779167 - Flags: review?(hurley)
Attachment #779167 - Flags: review+
Attachment #779167 - Flags: feedback?(doug.turner)
Attached patch patch v2Splinter Review
upped the cache limit to 250. I also made gOverrideForHostCache a Map rather than an object, as per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map#Objects_and_maps_compared
Attachment #779167 - Attachment is obsolete: true
Attachment #779167 - Flags: feedback?(doug.turner)
Attachment #779661 - Flags: feedback?(doug.turner)
Summary: UserAgentOverrides.jsm should cache the override (or that there's no override) for each host to avoid doing the same work for every HTTP request → UserAgentOverrides.jsm should cache the override (or that there's no override) for each host to avoid doing the same iterative string matching for every HTTP request
I went ahead and landed this so that I can continue working on bug 896114 and bug 891968.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b02a7e628050
Blocks: 891968
https://hg.mozilla.org/mozilla-central/rev/b02a7e628050
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Blocks: 788422
Attachment #779661 - Flags: feedback?(doug.turner)
Dao,

So this used to cause an 8% regression on desktop.  There's no perf info in this bug on how much you improved that (and so, for instance, it looks like we'll hack in a different, C++ solution in bug 1233970

Is there any chance you could benchmark this so we can figure out if UserAgentOverrides is now production-ready?
Flags: needinfo?(dao)
Perf has been tackled from various angles in this bug and related bugs. We've used this on desktop, b2g and android. It's very much production-ready from what I know. If somebody has claimed otherwise I'd like to know more before I spend more time on this.
Flags: needinfo?(dao)
You need to log in before you can comment on or make changes to this bug.