Closed
Bug 506471
Opened 15 years ago
Closed 14 years ago
FUEL should avoid putting itself in the app-startup category
Categories
(Firefox :: General, defect, P2)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 3.7a1
People
(Reporter: vlad, Assigned: dietrich)
References
Details
(Whiteboard: [ts])
Attachments
(1 file, 3 obsolete files)
11.31 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
FUEL places itself in the app-startup category so that it can notify Application clients that are listening to its 'load' event. At that point, the only things that could have added themselves are other components, and in the most common case, there's nothing listening. This patch saves about 20ms on a warm start; not quite sure why it takes that long for fuel to do its init work, but this lazily registers observers based on what events are actually listened on.
Attachment #390654 -
Flags: review?(mark.finkle)
Reporter | ||
Updated•15 years ago
|
Whiteboard: [ts]
Updated•15 years ago
|
Attachment #390654 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Updated•15 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Reporter | ||
Updated•15 years ago
|
Assignee: nobody → vladimir
Assignee | ||
Comment 1•15 years ago
|
||
anything else to do here, or is this checkin-needed?
Assignee | ||
Comment 2•15 years ago
|
||
done, per vlad on irc. http://hg.mozilla.org/mozilla-central/rev/28bf433cb225
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 3•15 years ago
|
||
Comment on attachment 390654 [details] [diff] [review] register observers lazily >diff --git a/toolkit/components/exthelper/extApplication.js b/toolkit/components/exthelper/extApplication.js >--- a/toolkit/components/exthelper/extApplication.js >+++ b/toolkit/components/exthelper/extApplication.js >... >@@ -565,23 +570,17 @@ extApplication.prototype = { > this._prefs = null; > this._extensions = null; > this._events = null; >+ this._registered = {}; > > this._info = Components.classes["@mozilla.org/xre/app-info;1"] > .getService(Ci.nsIXULAppInfo); > > var os = Components.classes["@mozilla.org/observer-service;1"] > .getService(Ci.nsIObserverService); Looks like this can be removed as well >- >- os.addObserver(this, "final-ui-startup", false); >- os.addObserver(this, "quit-application-requested", false); >- os.addObserver(this, "xpcom-shutdown", false); > },
Comment 4•15 years ago
|
||
Pushed http://hg.mozilla.org/mozilla-central/rev/1d8ea5bf9dbd to followup with a whitespace fix & remove the unused os mentioned in comment #3
Comment 5•15 years ago
|
||
What I committed.
Assignee | ||
Comment 6•15 years ago
|
||
backed out due to leaks on all platforms: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1255562807.1255564708.3209.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1255560357.1255563225.19271.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1255560357.1255563225.19271.gz
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•15 years ago
|
Priority: -- → P2
Assignee | ||
Updated•14 years ago
|
Assignee: vladimir → dietrich
Assignee | ||
Comment 8•14 years ago
|
||
updated. logs were gone, so pushed to tryserver. failing some tests as well as leaking: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1263522190.1263530712.15075.gz
Attachment #390654 -
Attachment is obsolete: true
Attachment #406323 -
Attachment is obsolete: true
Comment 9•14 years ago
|
||
I can't help but question whether this really needs to load during startup at all. Seems like making any consumers of this code load the component would be a better direction since it would save around 140 ms during startup on Firefox WinCE on the Tegra and the current approach will likely only save around half of that.
Comment 10•14 years ago
|
||
(In reply to comment #9) > I can't help but question whether this really needs to load during startup at > all. Seems like making any consumers of this code load the component would be a > better direction since it would save around 140 ms during startup on Firefox > WinCE on the Tegra and the current approach will likely only save around half > of that. Actually, this may very well be less than 70 ms savings (half of the savings if it is removed completely from the startup path) for the common case of no consumers of the code used in startup. More details are in bug 536504 and in the following blog post http://blog.mozilla.com/rstrong/2009/12/23/firefox-javascript-component-startup-costs/
Assignee | ||
Comment 11•14 years ago
|
||
(In reply to comment #9) > I can't help but question whether this really needs to load during startup at > all. Seems like making any consumers of this code load the component would be a > better direction That's what this patch does. It removes FUEL from app-startup category, which means the first time it gets instanciated is by a consumer. Also, it only listens for notifications for which callers have requested it to listen.
Comment 12•14 years ago
|
||
Thanks for the clarification... it wasn't clear that you also remove it from final-ui-startup from the comments / summary since they only mention app-startup.
Assignee | ||
Comment 13•14 years ago
|
||
Fixed the leak. We must listen for shutdown, in order to remove any other notifications, and call Utilities.free() in the /browser code.
Attachment #421768 -
Attachment is obsolete: true
Attachment #422917 -
Flags: review?(mark.finkle)
Updated•14 years ago
|
Attachment #422917 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 14•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/1ffccd3864e1
Status: REOPENED → RESOLVED
Closed: 15 years ago → 14 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•14 years ago
|
Target Milestone: --- → Firefox 3.7a1
Comment 15•14 years ago
|
||
Comment on attachment 422917 [details] [diff] [review] v3 > free : function() { >- this._bookmarks = null; >- this._livemarks = null; >- this._annotations = null; >- this._history = null; >- this._windowMediator = null; >+ this.bookmarks = null; >+ this.livemarks = null; >+ this.annotations = null; >+ this.history = null; >+ this.windowMediator = null; > } Does this actually work? I.e., do these have setters? Looks like they don't, this will fail, and should be "delete this.foo;" instead.
Assignee | ||
Comment 16•14 years ago
|
||
(In reply to comment #15) Dao filed bug 543672, and i patched there.
You need to log in
before you can comment on or make changes to this bug.
Description
•