The default bug view has changed. See this FAQ.

Lazily initialize engineMetadataService

RESOLVED FIXED in Firefox 3.7a1

Status

()

Firefox
Search
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: rflint, Assigned: rflint)

Tracking

({perf})

Trunk
Firefox 3.7a1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ts])

Attachments

(2 attachments, 1 obsolete attachment)

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

The engineMetadataService currently does two things for us on startup: it provides the engine order if it's changed from the default and allows us to determine if an engine is hidden or not.
Our current caching of the hidden attribute doesn't help us because we're not setting the actual value in most cases, which causes us to always check the eMS and the selected engine is set without using the order, so we can delay building the engine order until the user changes or manages their engines.
Attachment #358794 - Flags: review?(gavin.sharp)
Keywords: perf
Whiteboard: [TStartup]
Ryan - now that we are landing things on central again - is this something we should revive?  It doesn't have tests, but if you think there's a real Ts win here, it is worth landing, right?

I know it's waiting on a Gavin review, but I rather suspect this fell off-radar, and probably needs unrotting?
Minus the CACHE_VERSION change, this patch should still apply. It was a fairly decent win when I wrote it and I doubt the situation has changed much since - I'll have to get new numbers for it.
Test coverage is provided by the general suite for the search service (bug 458810), which actually did fall off the radar :)
Whiteboard: [TStartup] → [ts]
Created attachment 395767 [details] [diff] [review]
Branch version
Attachment #358794 - Attachment is obsolete: true
Attachment #395767 - Flags: review?(gavin.sharp)
Attachment #358794 - Flags: review?(gavin.sharp)
Attachment #395767 - Attachment description: Update CACHE_VERSION → Branch version
Attachment #395767 - Flags: review?(gavin.sharp)
Created attachment 403185 [details] [diff] [review]
Patch

I think we've been sitting on this win long enough - Paul, roping you in since you last touched most of what this patch is fiddling with (sorry!). Please feel free to grab me on IRC if you've got any questions. Thanks!
Attachment #403185 - Flags: review?(paul)
Flags: wanted-firefox3.6?
Comment on attachment 403185 [details] [diff] [review]
Patch

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

>+  set _sortedEngines(aValue) {
>+    this.__sortedEngines = aValue;
>+  },

Just change buildSortedEngines list to set __sortedEngines directly, and omit this setter?

>+      if (this.__sortedEngines) {
>         this._sortedEngines.push(aEngine);

I'd use __sortedEngines directly here as well.

>     this._sortedEngines = this._sortedEngines.concat(alphaEngines);
>+    return this._sortedEngines;

could collapse the assignment/return here.
Attachment #403185 - Flags: review+
Attachment #403185 - Flags: review?(paul)

Comment 6

8 years ago
Gavin, does this ease our story for multi-locale builds?
I don't think it has any impact on that. It doesn't touch engine loading at all.
http://hg.mozilla.org/mozilla-central/rev/fbf106437925
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3.6a1 → Firefox 3.7a1
Gavin - would this help us in Fennec too? If so, I think we should look for 192 approval.
Don't think it would help in Fennec, since it doesn't initialize the search service on startup.
Flags: wanted-firefox3.6?
You need to log in before you can comment on or make changes to this bug.