Closed Bug 336833 Opened 18 years ago Closed 18 years ago

microsummary service should delay caching generators until after bookmarks service inited

Categories

(Firefox Graveyard :: Microsummaries, defect)

2.0 Branch
x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: myk, Assigned: myk)

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 7 obsolete files)

To reclaim the slight Ts regression on sparky and atlantia from the microsummaries landing, the microsummary service should delay caching generators until after the bookmarks service has been initialized.
This patch makes the bookmarks service notify observers of the "bookmarks-service-initialized" topic when it finishes initializing the bookmarks data source.  The microsummary service then delays initialization until it receives that notification.
Assignee: nobody → myk
Status: NEW → ASSIGNED
Attachment #221034 - Flags: review?
Attachment #221034 - Flags: approval-branch-1.8.1?(bugs)
Comment on attachment 221034 [details] [diff] [review]
branch patch v1: makes microsummary service wait until it hears from bookmarks service

remove your profile-after-change observer too, since you don't seem to use it anymore
Attachment #221034 - Flags: review? → review-
Now that the service is observing bookmarks-service-initialized it doesn't need to be observing profile-after-change at all.
Attachment #221034 - Attachment is obsolete: true
Attachment #221034 - Flags: approval-branch-1.8.1?(bugs)
Attachment #221035 - Attachment is obsolete: true
Attachment #221036 - Flags: review?
Comment on attachment 221036 [details] [diff] [review]
patch v3: and this one breaks appropriately

r+a=ben@mozilla.org
Attachment #221036 - Flags: review?
Attachment #221036 - Flags: review+
Attachment #221036 - Flags: approval-branch-1.8.1+
Checked into the 1.8 branch.  Leaving the bug open until we get a trunk fix.

Brett, Ben says that "the 'delayedStartup' step is the query that populates the bookmarks toolbar" but that you would know best where in the Places code it makes the most sense to notify observers of the "bookmarks-service-initialized" topic.  What do you think?
Keywords: fixed1.8.1
I meant LoadBookmarks, not Init(). Oops.
Attachment #221036 - Attachment is obsolete: true
Myk, I landed this patch on the branch since you weren't on IRC. 
mconnor suggested on IRC that we actually wait until final-ui-startup "to prevent init before em restart", but it's not clear that doing so fixes the Ts regression.
So far I've had no luck getting a machine to give consistent Ts results.  Besides the general suggestions on the wiki (http://wiki.mozilla.org/Performance:Tinderbox_Tests#Notes_before_starting) and shutting down all unnecessary services, are there other things I can do to get consistent numbers I can use as a basis for testing potential solutions to this regression?
Here's another possible approach that not only waits until the bookmarks service has been initialized, it waits another second after that.  I considered waiting for final-ui-startup instead, but that happens before bookmarks service initialization, and since we can't do much without the bookmarks service, we might as wait for it.

It's unclear whether this patch helps fix this minor Ts regression, as my laptop lacks the resolution to make that determination, even after following the recommendations at http://wiki.mozilla.org/Performance:Tinderbox_Tests.  Also, it would probably be better to define a topic that definitively represents a "reached post-Ts state" notification and then observe that instead.  I'm just not sure where to fire off such a topic, and in the meantime this patch stands a reasonable chance of making a difference.

This patch also adds "bookmarks-service-initialized" notification to the Places version of the bookmarks service at the end of nsNavBookmarks::Init(), which is where Brett says it should go.
Attachment #221488 - Flags: review?
Attachment #221488 - Flags: review? → review?(bugs)
Attached patch patch v5 for the trunk (obsolete) — Splinter Review
This is a version of patch v5 that applies cleanly to the trunk, which is now out of sync with the branch since we checked in earlier patches only to the branch.
Per more discussions with Ben, this patch now initializes the microsummary service when delayedStartup() is called.
Attachment #221488 - Attachment is obsolete: true
Attachment #221490 - Attachment is obsolete: true
Attachment #221513 - Flags: review?(bugs)
Attachment #221488 - Flags: review?(bugs)
Comment on attachment 221513 [details] [diff] [review]
patch v6: initialize on delayedStartup()


>       case "xpcom-shutdown":
>+        this._obs.removeObserver(this, "xpcom-shutdown");
>         this._destroy();
>         break;

you don't need to explicitly remove this observer, since its holding a weak ref now it shouldn't leak (see bz's patch in bug 336922)
Attachment #221513 - Flags: review?(bugs) → review+
mconnor says on IRC to carry forward his review.  Note that when I check this in to the trunk I'll omit the change to nsBookmarksService.cpp, since that just reverts an earlier attempt to fix this bug that never made it to the trunk.
Attachment #221050 - Attachment is obsolete: true
Attachment #221513 - Attachment is obsolete: true
Attachment #221534 - Flags: review+
Comment on attachment 221534 [details] [diff] [review]
patch v7: includes the change to browser.js

sr+a=ben@mozilla.org
Attachment #221534 - Flags: superreview+
Attachment #221534 - Flags: approval-branch-1.8.1+
This has been landed on trunk and branch.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Component: Bookmarks → Microsummaries
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: