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.

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

Now that the service is observing bookmarks-service-initialized it doesn't need to be observing profile-after-change at all.

Comment on attachment 221036 [details] [diff] [review] patch v3: and this one breaks appropriately r+a=ben@mozilla.org

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?

I meant LoadBookmarks, not Init(). Oops.

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.

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.

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)

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.

Comment on attachment 221534 [details] [diff] [review] patch v7: includes the change to browser.js sr+a=ben@mozilla.org

This has been landed on trunk and branch.