1356593 - nsISearchEngine.speculativeConnect and nsISearchEngine.getSubmission are slow

Status: RESOLVED FIXED

(Firefox :: Search, enhancement, P1)

Description

[Florian Quèze [:florian]]

I got this profile by alternatively clicking the searchbar and location bar on a slow netbook: https://perfht.ml/2of8wvm

We spend a lot of time calling speculativeConnect() on the currentEngine. getSubmission is the slow part (and in getSubmission, getLocale is the slowest part).

We should:
- cache most of the submission URL the first time it's computed. Only the user's query terms change each time.
- cache the getLocale result the first time we get it (we get it while initializing the search service... and we do already have a pref observer that triggers an async reinitialization of the search service whenever the locale changes)
- we may want to add a fast path for getting only the prepath of the submission URL for the speculativeConnect code path.

Comment 1

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #0)

> - cache the getLocale result the first time we get it (we get it while
> initializing the search service... and we do already have a pref observer
> that triggers an async reinitialization of the search service whenever the
> locale changes)

Bug 1346616 changed the getLocale implementation yesterday to now just call something implemented in C++, so we may no longer need to cache this value in the search service.

Comment 2

[Florian Quèze [:florian]]

Attachment: reimplement ParamSubstitution

Looking more closely at ParamSubstitution showed that most of the time spent there was wasted. This patch is just a clean reimplementation of it that only does the work that's actually needed to get the same result (I wrote the test before the patch to ensure I wasn't changing the result; except for the {moz:official} parameter that I fixed; I had broken it with https://hg.mozilla.org/mozilla-central/rev/9c92165fef20#l7.16 ). It's likely to be enough and that the caching I suggested in comment 0 won't be needed.

I also replaced the makeURI call (that had overhead calling into NetUtils.jsm) in getSubmission with the simple Services.io.newURI, and I cleaned up the _getURLOfType implementation.

ParamSubstitution is 54% of the JS running in the profile in comment 0, and the makeURI call is 6.9%.

Comment 3

[Drew Willcoxon :adw]

Comment on attachment 8859773 [details] [diff] [review]
reimplement ParamSubstitution

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

This slipped through the cracks, sorry, LGTM.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68e88b3e0e57
reimplement the search service's ParamSubstitution function to avoid doing unnecessary work, r=adw.

Comment 5

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/68e88b3e0e57