Additional search service Ts improvements

RESOLVED FIXED in Firefox 3.1b2

Status

()

Firefox
Search
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: rflint, Assigned: rflint)

Tracking

({perf})

Trunk
Firefox 3.1b2
Points:
---
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Created attachment 343364 [details] [diff] [review]
Patch

- don't create an nsILocalFile for Engine::_file until needed
- cache Engine::_id so we're not poking file properties every startup
- call _isDefaultEngine for 'defaultEngine' mozparams only 
- eliminate the function invocation costs of ENSURE* for simple boolean checks
Attachment #343364 - Flags: review?(gavin.sharp)
Flags: wanted-firefox3.1?
Created attachment 343365 [details]
dtrace results
I like the idea of running less code, but it's hard to trade that off against source code complexity without knowing what kind of performance increase we're talking about here. In particular, I'm a bit doubtful that the ENSURE changes are worthwhile - is the function call overhead that significant? What portion of the speedup is attributable to that change?
20ms on ~1200 also doesn't seem to be very significant. Do you have a rough idea of how closely correlated previous dtrace-measured differences and Talos Ts wins have been?
(I don't mean to imply that we should only rely on Talos numbers, since they're not perfect either, but it's good to get as many perspectives as possible.)
In general I've found that the dtrace indicated savings are fairly close to what we'll see on Talos. 20ms may not seem like a lot in the grand scheme of things, but you have to remember that this isn't just a flat 20ms win - if a user has more than the default set of search plugins, they'll see a larger win. 20ms is also just shy of 30% of the total initialization cost of the search service which remains as the slowest front-end component we init up to the end of delayedStartup.

The ENSURE* changes account for nearly half of the win here. Function invocation overhead generally isn't a very juicy target, but the 125 combined calls during starup just make it irresistible low-hanging fruit :)
And again, the more search plugins loaded, the higher that particular win is.
Comment on attachment 343364 [details] [diff] [review]
Patch

Alright, that makes sense.

>diff -r d7c36a339ea2 toolkit/components/search/nsSearchService.js

Seems like we should use Components.Exception for ERROR and ENSURE_WARN as well, and there are a bunch of places where we currently just LOG and throw that could now use FAIL. Having both ERROR and FAIL is kind of confusing, but meh.

> function QueryParameter(aName, aValue) {
>-  ENSURE_ARG(aName && (aValue != null),
>-             "missing name or value for QueryParameter!");
>+  if (!aName || (aValue === null))
>+    FAIL("missing name or value for QueryParameter!");

Should catch both null and undefined, I think, so use "(aValue == null)"? An empty string being valid is the reason this wasn't just a simple (aName && aValue) check.
Attachment #343364 - Flags: review?(gavin.sharp) → review+
http://hg.mozilla.org/mozilla-central/rev/8f1ca9d63644
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
Flags: wanted-firefox3.5?
You need to log in before you can comment on or make changes to this bug.