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

RESOLVED FIXED in mozilla25

Status

()

Core
Networking
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

({perf})

unspecified
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

5 years ago
Keywords: perf
(Assignee)

Comment 1

5 years ago
Created attachment 779167 [details] [diff] [review]
hostCache.diff

Does this help?
Attachment #779167 - Flags: review?(doug.turner)
(Assignee)

Comment 2

5 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 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

5 years ago
Created attachment 779661 [details] [diff] [review]
patch v2

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

5 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

5 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
(Assignee)

Updated

5 years ago
Blocks: 891968

Comment 7

5 years ago
https://hg.mozilla.org/mozilla-central/rev/b02a7e628050
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(Assignee)

Updated

5 years ago
Blocks: 788422
(Assignee)

Updated

5 years ago
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)
(Assignee)

Comment 9

2 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.