Closed
Bug 896382
Opened 8 years ago
Closed 8 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)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: dao, Assigned: dao)
References
Details
(Keywords: perf)
Attachments
(1 file, 1 obsolete file)
2.40 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 2•8 years ago
|
||
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 3•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
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
Assignee | ||
Comment 6•8 years ago
|
||
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
Comment 7•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b02a7e628050
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Assignee | ||
Updated•8 years ago
|
Attachment #779661 -
Flags: feedback?(doug.turner)
Comment 8•5 years ago
|
||
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)
Assignee | ||
Comment 9•5 years ago
|
||
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.
Description
•