472343 - Managing multiple bookmarks in the Library is very slow

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Amit D Chaudhary]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.5) Gecko/2008120121 Firefox/3.0.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.5) Gecko/2008120121 Firefox/3.0.5

Hello,

I recently upgraded to Firefox 3.x. I found the following bug. It stayed even after disabling the bookmark extension I had.
Drag & Dropping bookmarks into another folder in same left pane is very slow, takes 5-7 seconds and all of firefox freezes over.

This did not happen with Firefox 2.x

Maybe the size of bookmarks matters, my places.sqlite is 13 MB+ and has 24,363 bookmarks. There are 1839 folders in the bookmarks, many of them about 6-9 level deep and about 10 folders at each level. The result of using netscape\mozilla\firefox for more than 8 years and collecting a lot and organizing.

I did not find any bug mention this as a new bug in 3.x, hence I filed this one

Reproducible: Always

Steps to Reproduce:
1.Open Organize bookmarks. Select a bookmark folder, about 2 levels deep with about 20 URL bookmarks and a few folders.
2. Select 5-10 bookmarks using mouse in the left pane
3. Drag them into another folder in the same left pane.

Actual Results:  
Everything will stop and it will take a while, about 5-7 seconds to start using firefox again.

Expected Results:  
About 1 second response time would be great.

Comment 1

[Nochum Sossonko [:Natch]]

Duping to bug 432706, if you can reproduce this on latest nightly builds, please reopen.

Comment 2

[Amit D Chaudhary]

Tried firefox-3.2a1pre, the build in nightly directory.

The performance is improved, but not fixed. It now takes approx 4 seconds for about 10 bookmarks dropped instead the earlier 6-7 seconds.

I am marking this as unconfirmed for now.

Please email me if you would like me to retry with another build.

Thanks
Amit

Comment 3

[Nochum Sossonko [:Natch]]

Fair enough, moving to places.

Comment 4

[Marco Bonardo [:mak]]

confirming for perf measurements and investigation

Comment 5

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I can also confirm this...
When I want to move about 200 or more bookmarks from 1 folder to another this operation take massive amount of time, in Opera this was in instant like I can remember ;p

Comment 6

[Marco Bonardo [:mak]]

when you report your experience (not that is really useful atm, since the bug is confirmed) you should always report on which version, since we have 3.0, 3.5, and development versions 3.6 and 3.7.

Comment 7

[Ivan Khlistov]

On FF 3.5.3 (Windows) removing 1K bookmarks takes about 2 minutes.
On FF 2.0 it takes 1 second.

Comment 8

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to comment #6)
> when you report your experience (not that is really useful atm, since the bug
> is confirmed) you should always report on which version, since we have 3.0,
> 3.5, and development versions 3.6 and 3.7.

confirmed on nightly version 3.7 in XP SP3

Comment 9

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 10

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Should we consider this for blocker ? Because major future is broken... if not for 3.6, place it for 3.7...
It's pain in the ass to manage bookmarks, moving about 300 from one folder to another hangs Fx for more than 30s on C2D E6550 2,33GHz@3,2GHz...

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Marco: do you have an idea of what's causing this? You say that the bug is confirmed, but I'm not experiencing the same problem (although I bet I don't have nearly the number of bookmarks that the reporter has)

Comment 12

[Marco Bonardo [:mak]]

It's caused by the fact we should profile the Places transactions manager and make it faster, adding some dedicated API.
This is by no means a regression from previous 3.x, instead 3.6 is quite faster than 3.5 that is faster than 3.0. Not as fast as we would like it to be yet, not as fast as 2.0, but 2.0 was handling far less data and differently, so i can't consider it a good comparison.
It can still happen with complex structures of bookmarks, and maybe interaction with some add-on, since when removing and recreating bookmarks we will notify all observers, and that can slowdown us.

Comment 13

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I do some test on my reinstalled Fx3.7a1pre (default settings, no addons) and see that without any favicons in imported html bookmark file, Fx is faster about 50-100% in bookmarks operation like moving more than 50-100 bookmarks to another folder or simply moving folder.

But odd, Chromium 4 Portable performs this operation even with favicons with imported html in instant, dunno on Opera cause it didnt import favicons with links, but without it, its instant operation too.

Do we need completely new database or simply some good optimizations ?
Cause in Fx2 moving bookmarks was fast like in Chromium or Opera, everything was moved in instant. (Tested some seconds ago on Fx2.0.0.14 from portableapps)

Comment 14

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Any changes to get this fixed in Fx 3.7 ?
Or at last will be nice to have it fixedf in Fx4, cause its pain in the ass to managing bookmarks in Fx

Comment 15

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Looking on this
"Fast: making Firefox super-duper fast"
on this website
http://beltzner.ca/mike/2010/05/10/firefox-4-fast-powerful-and-empowering/

Should this bug be considered a BLOCKER ? :)
Because now managing bookmarks in Firefox 3.0-3.7 is unacceptable
Fx hangs for some seconds when you move about 100-200 bookmarks
and when you move more than 1k, Firefox will hang for minutes

But in other browsers this is made in INSTANT...

Comment 16

[Dietrich Ayala (:dietrich)]

yes this should block fx4 final.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Dietrich, this needs an owner; do we have a plan for this?

Comment 18

[Marco Bonardo [:mak]]

While I understand the pain, I don't think that moving hundreds of bookmarks in a row or folders with deep sublevels (that ends up being hundreds of bookmarks again) is something that should block, _it's highly wanted_, but not being even a regression (instead it improved by 50%) I find an hard time in thinking to it as a blocker.

Comment 19

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I have about 5MB of bookmarks.html, why I need to wait about 1-2min to move a single folder with some tabs or use another browser for this simply task ?

It not happen with Fx 2.X, started with 3.X, so IMO it's clearly a regression and should be blocker for Fx4.
Because if it it won't be, we probably need to wait another years for fixing this.

Comment 20

[Marco Bonardo [:mak]]

bookmarks.html is not your bookmarks.

Comment 21

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I know that bookmarks and other info is stored in "places.sqlite", but I'm talking only about bookmarks (the exported ones).

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Does this bug only affect movements of hundreds of items, or does it affect:

 - folders that contain hundreds of items,
 - single items in large places.sqlite files?

If it's only happening when moving hundreds of selected items, I think we can unblock. If it's affecting moving large folders or single items within large files, I agree with Dietrich's blocking assertion.

Comment 23

[Marco Bonardo [:mak]]

the size of places.sqlite is a marginal problem here, the issue is the number of queries for each item.

Comment 24

[Shawn Wilsher :sdwilsh]

(In reply to comment #23)
> the size of places.sqlite is a marginal problem here, the issue is the number
> of queries for each item.
...and that can be caused by one large folder.  FWIW, we haven't blocked on this in 3.5 or 3.6 and it was a problem then (nominated even!).  It's not an easy problem to solve either and likely requires some major changes to the places schema.  This isn't the time in the release cycle where I'd be expecting to start this type of work.

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

I agree. I would have started this work in June, when it was made a blocker. UI responsiveness was always stated as a top goal for this release, and this bug languished, prioritized but not heeded to.

I'm loathe to unblock on it now, especially after being told that it's "hard". Let's understand:

 - what's causing it to be slow,
 - what would need to be done to make it faster,
 - whether we can fit that work in to the schedule remaining.

That we punted on this for previous releases - while often a decent guide - doesn't sway me here.

Comment 26

[Shawn Wilsher :sdwilsh]

This has largely the same set of issues as bug 432706, although this one is slightly more complicated as I understand it because our drag and drop code uses slightly different code paths.  It's slow because our architecture for this wasn't designed to handle large amounts of bookmarks or large bookmark folders.  It's been a design flaw from the start, and it's unfortunate that we took it in the first place.

mak can better comment on what needs to be done to make it faster, but it's going to be similar to bug 432706 (which had to get backed out in the 3.5 timeframe because of too many regressions).  It doesn't help that the UI bits aren't well tested here.

I won't comment on schedule until mak comments.

Comment 27

[Marco Bonardo [:mak]]

I can look into some piece of this next week. What we were supposed to do for this bug, or even import slowness, is to provide some API to read all information relative to a bookmark in a single call rather than in 4-5 separate calls, or even try to reuse data from the frontend nodes if available. The scope is to reduce queries traffic (Ondrej did something similar for import in 3.5).

I started revising the Places transaction manager in bug 553467 in June and next step was Bug 499458. This bug is mostly depending on that one.  Then other work took priority and it was temporarily put aside.

We can't solve slow observers or the underlying Places architectural flaws now, but reducing queries should help performances enough.

Comment 28

[Dietrich Ayala (:dietrich)]

If it's needed, do not hesitate to make read-only raw queries where necessary in order to fix this in the short term. I favor making this performant for millions of humans, over keeping the code pristine for us few.

Comment 29

[Marco Bonardo [:mak]]

I'm pushing a couple patches to tryserver. I'll really appreciate help in testing those, since automated tesst are nice, but they are far from covering 100% of the cases (so create a testing profile, copy your places.sqlite to it, then feel free to play in the Library). Also an evaluation of the perf improvement from current trunk would be nice.

I actually did not touch nor the transaction manager nor the queries, so looks like there is still space for future improvements.

This is where the builds _will_ appear, it will take some hour from now, especially for Windows builds.
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-4713fe227de4

Comment 30

[Marco Bonardo [:mak]]

Builds are available and awesome. D&D 140 bookmarks, with trunk it takes 4s with trybuild it takes 100ms.

I'm now going to do some functional test, please help me testing the Library, the builds won't stay there for more than a couple days.
If you notice any strange behavior tell me, I'll figure out if it regressed or is known.
I don't expect big regressions fwiw.

Comment 31

[Marco Bonardo [:mak]]

Also, all automated tests passed.

Comment 32

[Marco Bonardo [:mak]]

Attachment: part 1: clarify batch handling in results, avoid double refreshes

This part clarifies batches in results, instead of having nodes tracking the batches, it's the result that tracks them, when a node needs a refresh during a batch, it requests it to the result, that discards duplicate requests.

Comment 33

[Marco Bonardo [:mak]]

Attachment: part 2: make views aware of batches

This notifies the result observers when a batch starts or ends.
Result observers are the UI views, this is much more useful for trees (we don't do batch editing in menus or toolbars fwiw). When the tree notices a batch it can start his own batch and suppress selection events.

This is an API addition, but it's to a new API we introduced in 4.0 and is dedicated to views that are 100% js consumers. Thus being an addition I don't think it will hurt anyone. Will figure our someone for SR once it's reviewed.

Comment 34

[Marco Bonardo [:mak]]

ehr, please ignore the reportError I left in, will remove them in next version of the patch :)

Comment 35

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Moving about 1.000 bookmarks from one folder to another:
-without patch: about 3min
-with patch: about 3-5s
so it's better :)

But still Chromium and Opera perform this operation instantly with even 10.000 bookmarks in folder, odd...

Comment 36

[Marco Bonardo [:mak]]

Chromium holds all bookmarks in memory, that's why it's fast but taking much more memory and I doubt they have all the features we do, their bookmarking path is really simple. For example I have not find a way to undo a deletion, or to tag a bookmark. It's easy to be fast when you don't have to handle things.
Opera is closed source, so I can't tell you anything about them.

Btw, as I said above we still have space to improve, but this should be fine for the most common cases. I also don't want to load too many changes to a single bug and during final beta stage.

Finally, I'm happy that this improves greatly for you too! 3 minutes to 3 seconds sounds pretty good.

Comment 37

[Dietrich Ayala (:dietrich)]

Comment on attachment 488447 [details] [diff] [review]
part 1: clarify batch handling in results, avoid double refreshes


> NS_IMETHODIMP
> nsNavHistoryQueryResultNode::OnEndUpdateBatch()
> {
>-  NS_ASSERTION(mBatchInProgress, "EndUpdateBatch without a begin");
>-  // must set to false before calling Refresh or Refresh will ignore us
>-  mBatchInProgress = PR_FALSE;
>-  return Refresh();
>+  // Directly refreshing here would cause duped refreshes when something other
>+  // has already requested one.  Let the result handle duplicate requests.
>+  nsNavHistoryResult* result = GetResult();
>+  NS_ENSURE_STATE(result);
>+  result->requestRefresh(this);
>+
>+  return NS_OK;
> }

This comment only really makes in the context of the current behavior, which no longer exists once this patch lands. I'd remove it, or just describe what requestRefresh actually makes happen.

r=me

Comment 38

[Dietrich Ayala (:dietrich)]

Comment on attachment 488448 [details] [diff] [review]
part 2: make views aware of batches

removing those debugging calls and add a test for the new api, and this'll be r+'able.

Comment 39

[Marco Bonardo [:mak]]

Attachment: part 2: make views aware of batches

Comment 40

[Marco Bonardo [:mak]]

Attachment: part 3: merge batches logic

I want to merge the logic for bookmarks and history batches. Since now we don't serve duplicated Refreshes, there is no reason to keep having 2 batches.
Plus that creates weird edge cases where:
- a result cannot already distinguish which batch started/ended.
- a result most likely does not need to distinguish, indeed we never do.
- we could be setting mBatchInProgress for a history batch and it could be unset from a bookmarks batch, or it could even be never unset. Handling this correctly would add a bunch of code to check the kind of batch and the kind of observer, leaving a lot of confusion for observers being both.
- it's simpler and less confusing.

both part 2 and part 3 are using the same test, that is fine to test the full behavior. in practice it tests that batching is caught in both cases that it is coming from history or bookmarks service.

Comment 41

[Marco Bonardo [:mak]]

Attachment: part 1 (v1.1): clarify batch handling in results, avoid double refreshes

Comment 42

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 489860 [details] [diff] [review]
part 2: make views aware of batches

Please add relnote tag and describe the IDL change

Comment 43

[Marco Bonardo [:mak]]

for the relnote:

Added a batching(aToggleMode) method to nsINavHistoryResultObserver. the method notifies history and bookmarks views when a batch (heavy and on multiple items) operation starts and ends. Views can temporarily disable stuff like selection events during that time, for performance reasons and to avoid flickering.

Comment 44

[Marco Bonardo [:mak]]

part 1: http://hg.mozilla.org/mozilla-central/rev/e2ad39636491
part 2: http://hg.mozilla.org/mozilla-central/rev/4503884294a7
part 3: http://hg.mozilla.org/mozilla-central/rev/1911a8629c39

Comment 46

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/nsINavHistoryResultObserver#batching()

And mentioned on Fx4 for developers:

https://developer.mozilla.org/en/Firefox_4_for_developers#Interface_changes

Comment 47

[Marco Bonardo [:mak]]

thanks, I've slightly modified the documentation since it was backwards (this is not a method used by the observer to notify to Places, but a method used by Places to notify the observer).

Comment 48

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

OK, thank you!
Landed and works, but is there any tracing bug for speeding Library more like in Chromium or Opera ?

Comment 49

[Marco Bonardo [:mak]]

(In reply to comment #48)
> Landed and works, but is there any tracing bug for speeding Library more like
> in Chromium or Opera ?

So, by dropping features? no.

But bug 499458 can help us getting faster, and we can do other small polish cleanups here and there that will help. Also bug 552023 will help here.
So, there are various perf bugs around the will globally help, not all of them will make FX4, some will do.

Comment 50

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to comment #49)
> (In reply to comment #48)
> > Landed and works, but is there any tracing bug for speeding Library more like
> > in Chromium or Opera ?
> 
> So, by dropping features? no.

Which features are you have in mind ?

Comment 55

[[e-mail]]

Comment on attachment 489860 [details] [diff] [review]
part 2: make views aware of batches

>+  batching: function PTV__batching(aToggleMode) {
This would normally be written something like this:
setBatching: function PTV_setBatching(aBatching) {
  if (this.selection.selectEventsSuppressed != aBatching) {
    this.selection.selectEventsSuppressed = aBatching;
    if (aBatching)
      this._tree.beginUpdateBatch();
    else
      this._tree.endUpdateBatch();
  }
}
[I don't think you can have a tree yet not have a selection.]

>+  void batching(in boolean aToggleMode);
[Since write-only attributes don't exist I would have named this setBatching]

Comment 56

[[e-mail]]

(In reply to comment #55)
>   if (this.selection.selectEventsSuppressed != aBatching) {
Although in your existing code you don't actually do this check when turning on batching, so maybe this line is unnecessary.

Comment 57

[Marco Bonardo [:mak]]

please file new bugs for suggested improvements, otherwise they are going to get lost in the middle of bugmails.