629742 - General browser performance seems bad with lots of Live Bookmarks

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Les Orchard [:lorchard]]

Attachment: My Google Reader subscriptions, hopefully somewhat sanitized

Steps to reproduce:

1) Get an OPML export of lots of feed subscriptions (attached is mine from Google Reader, containing ~700 feeds)

2) Install the OPML Support addon - https://addons.mozilla.org/en-US/firefox/addon/opml-support/

3) Import the set of subscriptions as Live Bookmarks using OPML Support. Use default options, make sure folders are created and that Live Bookmarks are created. This will take awhile, and may cause a script stop dialog to appear. Just let it run.

4) Live Bookmark refreshes will happen automatically, but you can force the issue with the Relibly addon to reload all Live Bookmarks - https://addons.mozilla.org/en-US/firefox/addon/reliby/

Over time, I get lots of freezes and beach-balls, scrolling pages is unresponsive and typing in forms is difficult. That all stops when I disable sync.

My guess is that sync is picking up all the bookmarks automatically generated within Live Bookmarks. Given 700 feeds and ~15 items within each, that's over 10000 bookmarks.

FWIW: The way nsLivemarkService.js works is that each Live Bookmark is a folder. When the associated feed is checked, all bookmarks in the folder are deleted, and new ones are created for each item in the latest feed fetch. Lots of bookmark thrash there.

And... since I disabled this device to get things usable again, it reset all sync info. I'll try reproducing all the above so I can attach a log, but I'm filing this bug for now.

Comment 1

[Richard Newman [:rnewman]]

(In reply to comment #0)

> My guess is that sync is picking up all the bookmarks automatically generated
> within Live Bookmarks. Given 700 feeds and ~15 items within each, that's over
> 10000 bookmarks.

The bookmark tracker has code which purports to ignore livemarks, so it shouldn't be actually tracking the contents, but I believe there are known issues in this area, so who knows.

Even so, the tracker does a surprising amount of work on each onItemAdded call (e.g., querying annotations to make sure we have a Mobile Bookmarks folder!), which is bound to impact performance if we're still handling those callbacks.

Should be easy enough to repro. Thanks for the thorough STR!

Comment 2

[Richard Newman [:rnewman]]

Oh, and another question: is there any performance impact for you with Sync disabled? (I.e., in Prefs, the Sync pane shows "Set Up Sync".)

Comment 3

[Les Orchard [:lorchard]]

(In reply to comment #2)
> Oh, and another question: is there any performance impact for you with Sync
> disabled? (I.e., in Prefs, the Sync pane shows "Set Up Sync".)

Nope - once I disabled sync, everything got snappy again.

Comment 4

[Richard Newman [:rnewman]]

I reproduced this. Several findings:

* OPML import is dog slow, partly because of the stack of notifications we get for every single addition:

13:00:50  Tracker.Bookmarks   Adding changed ID: o0IHSZVyOhfu,1296680450
13:00:50  Tracker.Bookmarks   onItemChanged: 4287, livemark/feedURI (anno)
13:00:50  Tracker.Bookmarks   onItemChanged: 4287, livemark/siteURI (anno)
13:00:50  Tracker.Bookmarks   onItemChanged: 4287, bookmarkProperties/description (anno)

and partly because we occasionally sync mid-import. Not much we can do about that.


* Yes, the browser is sluggish afterwards. I suspect this has a few causes:

  * Periodic updates apparently prompts disk IO. That's no fun.

13:16:07  Tracker.Bookmarks   Adding changed ID: 15fdQwKUSs6c,1296681367
13:16:08  Tracker.Bookmarks   Saving json to disk: weave/changes/bookmarks.json
13:17:46  Tracker.Bookmarks   onItemChanged: 2700, livemark/siteURI (anno)
13:17:46  Tracker.Bookmarks   Adding changed ID: MAf3fKRF8Yd4,1296681466
13:17:47  Tracker.Bookmarks   Saving json to disk: weave/changes/bookmarks.json
13:21:03  Tracker.Bookmarks   onItemChanged: 2761, livemark/siteURI (anno)
13:21:03  Tracker.Bookmarks   Adding changed ID: dWbCpGlpMFN1,1296681663
13:21:04  Tracker.Bookmarks   Saving json to disk: weave/changes/bookmarks.json

  * about:memory reports a huge amount of memory in use -- an empty window with
    Slashdot, about:config, and about:memory open shows 388,833,280 mapped and
    229,385,424 in use. Yow. 5MB is sqlite page cache; the rest is all zone0
    allocated memory.

Looking deeper now.

Comment 5

[Richard Newman [:rnewman]]

OK, memory seems to be a red herring, as does some of the performance issue. If I kick off a Live Bookmarks refresh with Sync disabled, the browser becomes unusable, and it sucks down over 600MB of RAM with no tabs open.

That is, Live Bookmarks by themselves are able to screw heavy users. Reliby locked up Minefield and set my fans to spinning as it chewed up 100% of one processor. (Phrased differently: "livemarks suck". Or, "Firefox is not a feed reader".)

My profile using Instruments with Sync enabled showed that SAX parsing was responsible for half the allocated memory, and the characteristic sawtooth pattern of GCs was evident in the rest. JavaScript chews through memory, and there's not much we can do about that; spacing between GCs allows our memory footprint to balloon to a certain extent. There don't seem to be any leaks or swelling globals in the bookmark engine.


Given the above, the area I'm concerned with is the more usual background updating and Sync's effect on performance. Live Bookmark changes cause onItemAdded/onItemChanged notifications to be received by Sync, and these involved (unfortunately) quite a lot of work... particularly as adding one item typically involves *four* such notifications.

A change I have in my local tree cuts the processing time for one of these notifications from 50ms to 1ms -- dropping the processing time for 10,000 items from 2,000 seconds to 40 -- and should reduce disk IO, which will help perceived performance. Unfortunately, to handle one of those notifications still requires:

* Asking Places for parents
* Checking whether something is a tag, in the tags folder, or a Live Bookmark
* Asking the annotation service for an anno

The only way to really avoid lagging the UI during Live Bookmark updates is to not receive the notifications, and perform all of the necessary legwork through database queries during sync itself. That's definitely a task for a future release.

In the mean time, I'll roll up a minor efficiency patch and get it tested, and we can all hope for a swift resolution for Bug 622049!

Comment 6

[Les Orchard [:lorchard]]

(In reply to comment #5)
> OK, memory seems to be a red herring, as does some of the performance issue. If
> I kick off a Live Bookmarks refresh with Sync disabled, the browser becomes
> unusable, and it sucks down over 600MB of RAM with no tabs open.
> 
> That is, Live Bookmarks by themselves are able to screw heavy users. Reliby
> locked up Minefield and set my fans to spinning as it chewed up 100% of one
> processor. (Phrased differently: "livemarks suck". Or, "Firefox is not a feed
> reader".)

FWIW, I've stopped using Reliby and have been just letting livemarks update themselves periodically. Seems like the reloadAllLivemarks from "@mozilla.org/browser/livemark-service;2" really does some damage, whether sync's enabled or not.

Oddly enough, I *am* using Firefox as a feed reader [1], which is weird and probably doomed to failure. But, I haven't noticed any slowdowns since I disabled sync. Memory usage on the other hand...

[1]: https://addons.mozilla.org/en-US/firefox/addon/fireriver/

...

> In the mean time, I'll roll up a minor efficiency patch and get it tested, and
> we can all hope for a swift resolution for Bug 622049!

Actually, I hadn't seen that bug before, and it makes me rather sad.

Comment 7

[Richard Newman [:rnewman]]

Les, if you're feeling inclined there's an XPI at

  http://people.mozilla.com/~rnewman/lmorchard.xpi

It'll install over Minefield or b10 (or 3.6, if you wish). Remember to uninstall it when you're done :D

See if this improves the browsing experience for you. Feedback welcome.

Comment 8

[Richard Newman [:rnewman]]

(In reply to comment #6)

> Oddly enough, I *am* using Firefox as a feed reader [1], which is weird and
> probably doomed to failure. But, I haven't noticed any slowdowns since I
> disabled sync. Memory usage on the other hand...

Yeah, I saw FireRiver; neat! Perhaps it's fairer to say "Places and its event model are not a good backing store for a feed reader" :)

Me, I used to manage 400 or so feeds in NetNewsWire; about a year ago I switched to Google Reader (with Reeder on my iPad), and I've been happy with that. Coping with multi-device access and syncing was just too annoying.

Re memory usage: Firefox usually hovers around 900-1200MB RSIZE, 5GB VSIZE just to cope with my open tabs, so adding more to that would be out of the question for me :D

 
> Actually, I hadn't seen that bug before, and it makes me rather sad.

There's still a lot of contention around it (and its siblings: Bug 622047 and Bug 622045), but it's good to think about these things, regardless of the outcome. Live/Smart/Tag bookmarks have a huge pile of maintenance burden, API cruft, challenges for features like Sync, and undiscoverable UIs. One has to ask whether it's worth occupying, say, sdwilsh's time for a month for each release, just to maintain a feature that a tiny fraction of users are even aware of.

Ah, the twists and turns of software planning.

Comment 9

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

This patch is the core of the XPI to which I just linked.

It's passed CrossWeave and tests, but I don't think those heavily exercise livemarks or other esoteric features. Consider this preliminary.

Comment 10

[Richard Newman [:rnewman]]

Nominating, in case mconnor wants to prioritize this.

Comment 11

[Mike Connor [:mconnor]]

Not a blocker, will take a reviewed+well tested patch.

Comment 12

[Richard Newman [:rnewman]]

Les, how's the performance in RC2? We made some changes which could affect performance, even without the improvements in the attached patch.

Comment 13

[Les Orchard [:lorchard]]

(In reply to comment #12)
> Les, how's the performance in RC2? We made some changes which could affect
> performance, even without the improvements in the attached patch.

Better, but not great. Still lots of general UI freezes and delays.

I reinstalled my addon, reimported the livemarks and let it stew for an hour or two. Performance was pretty bad while an overall livemark refresh was happening, and not-so-great when scheduled refreshes happened. Going to uninstall the addon and delete the livemarks for now

Comment 14

[Richard Newman [:rnewman]]

(In reply to comment #13)

> Better, but not great. Still lots of general UI freezes and delays.

OK, that makes sense. Thanks for checking.

There's a limit to how good we can get here -- our change observer gets blindly called for every bookmark, including livemarks -- so the question is how much better things get if we drastically shorten the abort path in the observer. (That's what the patch does.)

I'll circle back on this after I take care of some things (and Philipp takes a look at the patch!).

Comment 15

[Les Orchard [:lorchard]]

(In reply to comment #14)
> (In reply to comment #13)
> 
> > Better, but not great. Still lots of general UI freezes and delays.
> 
> OK, that makes sense. Thanks for checking.
> There's a limit to how good we can get here 
...

FWIW, I've since become convinced that I don't really want to build a feed reader out of livemarks. Probably something in a little node.js server is more appropriate.

But, feel free to snag me for additional stress tests. I seem to have a knack for abusing things, just ask telliott :)

Comment 16

[Richard Newman [:rnewman]]

> FWIW, I've since become convinced that I don't really want to build a feed
> reader out of livemarks. Probably something in a little node.js server is more
> appropriate.

Probably, but we still don't want to suck during LM updates! :D

I'm updating the patch now, so I can see how things behave.

Comment 17

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v2

I believe this is an accurate modern recasting of last month's patch :)

Yes, code written on Release Day!

Uploading this so I don't forget about it, but not flagging the old version as obsolete until I've done more testing. It applies, runs, Livemarks refresh, and Sync completes, but it almost seems like Livemark refreshes don't ever call my change handler... and the evil refreshing add-on pretty much locks up the browser.

Comment 18

[Philipp von Weitershausen [:philikon]]

Comment on attachment 521094 [details] [diff] [review]
Proposed patch. v2

>+      // Allocate a new GUID if necessary.
>+      // We only want to do it if there's a dupe, so use idForGUID to achieve that.

Actually, that bit isn't necessary anymore in Fx4

Comment 19

[Richard Newman [:rnewman]]

Removed some 3.6 stuff and pushed:

https://hg.mozilla.org/services/services-central/rev/4ad9a3906f71

Comment 20

[Philipp von Weitershausen [:philikon]]

Comment on attachment 521094 [details] [diff] [review]
Proposed patch. v2

>+    if (isAnno) {
>+      // Ignore annotations except for the ones that we sync.
>+      if (ANNOS_TO_TRACK.indexOf(anno) == -1)

Where is 'anno' defined? Should probably use 'property' instead...

Also, why the heck are tests passing? We must be not exercising that code path. :(

Comment 21

[Richard Newman [:rnewman]]

Attachment: Fix for the dumb. v1

Comment 22

[Richard Newman [:rnewman]]

(In reply to comment #21)
> Created attachment 524528 [details] [diff] [review]
> Fix for the dumb. v1

Pushed:

https://hg.mozilla.org/services/services-central/rev/e35d87f188eb

Comment 23

[Philipp von Weitershausen [:philikon]]

STRs for QA: When refreshing lots of Livemarks at the same time, the browser should no longer lock up. Problem is that it's pretty much impossible to trigger this manually through the UI, it would have to be an extension triggering the simultaneous Livemark updates. Marking [qa-] for that reason.

Comment 24

[Philipp von Weitershausen [:philikon]]

http://hg.mozilla.org/mozilla-central/rev/4ad9a3906f71
http://hg.mozilla.org/mozilla-central/rev/e35d87f188eb