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

Status: RESOLVED FIXED

(Core :: Networking, defect)

Comment 1

[Dão Gottwald [::dao]]

Attachment: hostCache.diff

Does this help?

Comment 2

[Dão Gottwald [::dao]]

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

[Jason Duell]

Comment on attachment 779167 [details] [diff] [review]
hostCache.diff

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

giving this review to Nick

Comment 4

[u408661]

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

Comment 5

[Dão Gottwald [::dao]]

Attachment: 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

Comment 6

[Dão Gottwald [::dao]]

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

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b02a7e628050

Comment 8

[Jason Duell]

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?

Comment 9

[Dão Gottwald [::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.