Closed Bug 472343 Opened 16 years ago Closed 14 years ago

Managing multiple bookmarks in the Library is very slow


(Firefox :: Bookmarks & History, defect)

Not set



Tracking Status
blocking2.0 --- final+


(Reporter: amitch, Assigned: mak)


(Blocks 1 open bug)


(Keywords: dev-doc-complete, relnote, Whiteboard: [TSnappiness][comment 43 for relnote/docs on the idl change])


(3 files, 2 obsolete files)

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


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.
Version: unspecified → 3.0 Branch
Whiteboard: DUPEME
Duping to bug 432706, if you can reproduce this on latest nightly builds, please reopen.
Closed: 16 years ago
Resolution: --- → DUPLICATE
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.

Resolution: DUPLICATE → ---
Fair enough, moving to places.
Component: Bookmarks & History → Places
QA Contact: bookmarks → places
Whiteboard: DUPEME
Whiteboard: [TSnappiness]
confirming for perf measurements and investigation
Ever confirmed: true
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
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.
On FF 3.5.3 (Windows) removing 1K bookmarks takes about 2 minutes.
On FF 2.0 it takes 1 second.
(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
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.

Component: Places → Bookmarks & History
QA Contact: places → bookmarks
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...
Flags: blocking-firefox3.6?
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)
blocking2.0: --- → ?
Flags: wanted-firefox3.6+
Flags: blocking-firefox3.6?
Flags: blocking-firefox3.6-
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.
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)
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
Looking on this
"Fast: making Firefox super-duper fast"
on this website

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...
yes this should block fx4 final.
blocking2.0: ? → final+
Dietrich, this needs an owner; do we have a plan for this?
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.
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.
bookmarks.html is not your bookmarks.
I know that bookmarks and other info is stored in "places.sqlite", but I'm talking only about bookmarks (the exported ones).
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.
the size of places.sqlite is a marginal problem here, the issue is the number of queries for each item.
(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.
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.
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.
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.
Depends on: 499458
Assignee: nobody → mak77
Whiteboard: [TSnappiness] → [TSnappiness][investigating short term solution for 4.0]
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.
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.
Whiteboard: [TSnappiness][investigating short term solution for 4.0] → [TSnappiness][patch in progress]
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.
Also, all automated tests passed.
Keywords: main-thread-io
OS: Linux → All
Hardware: x86 → All
Whiteboard: [TSnappiness][patch in progress] → [TSnappiness][please help testing builds in comment 29]
Version: 3.0 Branch → Trunk
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.
Attachment #488447 - Flags: review?(dietrich)
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.
Attachment #488448 - Flags: review?(dietrich)
No longer depends on: 499458
ehr, please ignore the reportError I left in, will remove them in next version of the patch :)
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...
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 on attachment 488447 [details] [diff] [review]
part 1: clarify batch handling in results, avoid double refreshes

> 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.

Attachment #488447 - Flags: review?(dietrich) → review+
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.
Attachment #488448 - Flags: review?(dietrich) → review-
Attachment #488448 - Attachment is obsolete: true
Attachment #489860 - Flags: review?(dietrich)
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.
Attachment #489863 - Flags: review?(dietrich)
Flags: in-testsuite?
Whiteboard: [TSnappiness][please help testing builds in comment 29] → [TSnappiness]
Summary: Drag & Dropping bookmarks is very slow, takes 5-7 seconds → Managing multiple bookmarks in the Library is very slow
Attachment #489860 - Flags: review?(dietrich) → review+
Attachment #489863 - Flags: review?(dietrich) → review+
Attachment #489860 - Flags: superreview?(vladimir)
Comment on attachment 489860 [details] [diff] [review]
part 2: make views aware of batches

Please add relnote tag and describe the IDL change
Attachment #489860 - Flags: superreview?(vladimir) → superreview+
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.
Keywords: relnote
Whiteboard: [TSnappiness] → [TSnappiness][comment 43 for the relnote on the idl change]
part 1:
part 2:
part 3:
Closed: 16 years ago14 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b8
Keywords: dev-doc-needed
Whiteboard: [TSnappiness][comment 43 for the relnote on the idl change] → [TSnappiness][comment 43 for relnote/docs on the idl change]
Target Milestone: Firefox 4.0b8 → ---
Depends on: 612235
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).
OK, thank you!
Landed and works, but is there any tracing bug for speeding Library more like in Chromium or Opera ?
(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.
(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 ?
Depends on: 620198
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)
[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]
(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.
please file new bugs for suggested improvements, otherwise they are going to get lost in the middle of bugmails.
You need to log in before you can comment on or make changes to this bug.