692120 - The star button doesn't need to observe bookmarks changes till bookmarks service is alive

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

The star button observes bookmarks changes, but to do so it inits the service, it should rather start observing when the service starts since there is no way bookmarks may change before.
Bookmarks service notifies bookmarks-observers category, we may try to hook there.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: wip 1.0

just a checkpoint, still not working correctly.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

This one works fine, with this patch and the one in Bug 692119 there is only one connection clone left in the startup path, that is the isVisited one. Plus we don't do anymore a bunch of work needed to init bookmarks service and the autocomplete connection.

I added 2 new helpers to PlacesUtils to lazily observe bookmarks. When a bookmarks notification is sent the PlacesCategoryStarter component catches it and notifies PlacesUtils. At ths point the observer is really added and notified (this relies on the fact categories are notified before common observers in Places).
I used these in the StarButton and in Livemarks service since both should not init the service unless needed.
I also removed the cloned connection from bookmarks service, I originally added it because bookmarks service init was happening in the startup path, but this is not the case anymore, plus I want to reimplement that in a centralized way in bug 689894.

Now going to Try to check tests and Talos.

Comment 3

[Dietrich Ayala (:dietrich)]

Comment on attachment 565253 [details] [diff] [review]
patch v1.0

Review of attachment 565253 [details] [diff] [review]:
-----------------------------------------------------------------

r=me w/ comments addressed.

::: toolkit/components/places/PlacesUtils.jsm
@@ +326,5 @@
> +        break;
> +      case "bookmarks-service-ready":
> +        Services.obs.removeObserver(this, "bookmarks-service-ready", false);
> +        while (this._bookmarksServiceObserversQueue.length > 0) {
> +          let observer = this._bookmarksServiceObserversQueue.shift();

since you're shift()ing here, won't these entries no longer be there at shutdown when you remove the lazy observers?

@@ +2146,5 @@
> +   * alive before registering the observer.  This is especially useful in the
> +   * startup path, to avoid initializing the service just to add an observer.
> +   *
> +   * @param aObserver
> +   *        Object imeplementing nsINavBookmarkObserver

nit: typo

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

> ::: toolkit/components/places/PlacesUtils.jsm
> @@ +326,5 @@
> > +        break;
> > +      case "bookmarks-service-ready":
> > +        Services.obs.removeObserver(this, "bookmarks-service-ready", false);
> > +        while (this._bookmarksServiceObserversQueue.length > 0) {
> > +          let observer = this._bookmarksServiceObserversQueue.shift();
> 
> since you're shift()ing here, won't these entries no longer be there at
> shutdown when you remove the lazy observers?

if on shutdown the queue has any entry, that just means the service never started, then we just empty the queue to lose ownership. The queue is used only till the service goes live and when that happens is depleted (through that shift)

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

this improves Ts but regresses Ts_shutdown (included dirty profiles), I have no idea why though, I tried launching the browser and closing it, and without the patch we init bookmarks service, with the patch we don't, so there is definitely less work to do on startup and on shutdown. So either Ts uses some fancy prefs that inits bookmarks service later, or something sneaks into the freed up space and ends up being accounted on shutdown. If this would be real tp5 shutdown would regress as well, that's not the case.
If you have ideas regarding which part of the patch may be culprit let me know, I'll setup standalone talos again in the meanwhile.
http://tinyurl.com/66utqk2

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Checking PRLog during Ts/Ts_shutdown, with the patch we run less queries, all of the bookmarks service initialization queries are correctly missing, so nothing is initializing it later.
My hypothesis is that the Ts win causes Ts_shutdown to start earlier and include some workload that before was being completed during Ts or the time between Ts and Ts_shutdown. Initializing bookmarks service was indeed blocking the main-thread while other threads could proceed, so only a part of the work could have been logged on shutdown.
Moreover, running Ts_shutdown locally I see an improvement and not a regression, while Ts is stable, that confirms this measurement is time-sensitive.
I'm going to do another couple measures on Try, but I'm confident enough this is not a real regression.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

So, I've tried to:
- disable expiration completely
- disable expiration and jumplists (they run a sync query to figure if there is any history)
- disable expiration and jumplists and frecency updates

The result is that there is no change in shutdown, the regression is still identical according to Try and Places is not doing any shutdown work apart finalizing a couple statements and closing the connection.

I think at this point the only way on is to get explicit approval to land as-is and get a new baseline for Ts (improved) and Ts_shutdown (regressed).
Dietrich?

Comment 8

[Dietrich Ayala (:dietrich)]

How can we confirm the theory in comment #6?

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Dietrich Ayala (:dietrich) from comment #8)
> How can we confirm the theory in comment #6?

I have no idea, since I cannot reproduce anything locally and if we do more work that's clearly not in Places, since I commented out all Places shutdown work in comment 7 and nothing changed. The only left work are visit and favicon addition, I may try to also disable those, since they are async they may happen at any time.
The only thing that happened for sure was that initing the star was blocking main-thread, due to bookmarks service initialization and this does not happen anymore. Ts happens earlier, as numbers show, then it invokes quit-application-requested and quit-application-granted and then tries to close the window. Some non-mainthread work could absolutely proceed while it was blocked, while it would now happen on xpcom-shutdown-threads. But I can't think of a way to demonstrate that.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Completely disabling visits and favicon didn't help as well, the additional accounted time is not in Places.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

bug 693645 is trying to add an additional timestamp to Ts so that one can measure TsRuntime and distinguish real regressions from fake ones.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

A brief calculation, accounting for noise in Ts and Shutdown and the sum of both effects, shows that linux and osx are in the expected noise, windows XP has a small 10ms increase, that may even be some not accounted noise. The problematic case is win7, with a 50-80ms difference, I wonder if jumplists have the time to sneak into the test timeframe and fire a history query, or if it is just a thread scheduler thing. Btw, if there would be a real increase due to this patch, it would be visible on more than a single platform considering the nature of the patch.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Checking Truntime on Windows, thanks to the new Talos timestamps, confirms an increase, without the patch Truntime is about 1250ms, with the patch it's around 1300ms. So, this should confirm something is doing more work, the problem is that for sure it's not something directly due to this patch.
An interesting fact is that Ts_shutdown uses setTimeout(0) to start shutdown, with bookmarks blocking mainthread that may have been enqueued earlier, not leaving space for something other, without bookmarks blocking mainthread ideally something may be inited before shutdown event is enqueued, hard to demonstrate tough.
2 remaining tests I'm going to do: disable idle service and disable jumplists.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Marco Bonardo [:mak] from comment #13)
> 2 remaining tests I'm going to do: disable idle service and disable
> jumplists.

That didn't help, I finished ideas.

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d659cf02d6b

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1d659cf02d6b