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)

defect

Tracking

()

RESOLVED FIXED
Firefox 3.7a1

People

(Reporter: vlad, Assigned: dietrich)

References

Details

(Whiteboard: [ts])

Attachments

(1 file, 3 obsolete files)

Attached patch register observers lazily (obsolete) — 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)
Attachment #390654 - Flags: review?(mark.finkle) → review+
OS: Mac OS X → All
Hardware: x86 → All
Assignee: nobody → vladimir
anything else to do here, or is this checkin-needed?
done, per vlad on irc.

http://hg.mozilla.org/mozilla-central/rev/28bf433cb225
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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);
>   },
Pushed http://hg.mozilla.org/mozilla-central/rev/1d8ea5bf9dbd to followup with a whitespace fix & remove the unused os mentioned in comment #3
Attached patch followup (obsolete) — Splinter Review
What I committed.
Priority: -- → P2
Assignee: vladimir → dietrich
Attached patch register observers lazily + more (obsolete) — Splinter Review
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
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.
(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/
(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.
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.
Attached patch v3Splinter Review
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)
Attachment #422917 - Flags: review?(mark.finkle) → review+
http://hg.mozilla.org/mozilla-central/rev/1ffccd3864e1
Status: REOPENED → RESOLVED
Closed: 15 years ago14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
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.
Depends on: 543672
(In reply to comment #15)

Dao filed bug 543672, and i patched there.
No longer blocks: FF2SM
You need to log in before you can comment on or make changes to this bug.