357080 - Camino perf is very slow when deleting a large number of bookmarks

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[froodian (Ian Leue)]

When deleting a large number of bookmarks, Camino appears to hang.  This is most clearly visible when trying to delete an obscene number of bookmarks, but there is perceivable sluggishness at anything > 10 bookmarks, and somewhere around > 30 it starts beachballing for me.  STR:

1. Import the bookmarks file found in bug 236373
2. Cmd-B
3. select the imported bookmarks collection
4. delete

Results:Beahball

Comment 1

[froodian (Ian Leue)]

Attachment: Sample of Camino Beachballing

Comment 2

[Stuart Morgan]

That stack is horked, but it's likely that the notification part is accurate given my experience with loading. We should look into batching removal notificiations for mass removal.

Comment 3

[Simon Fraser [no longer active]]

Yup, batching would fix this.

Comment 4

[Håkan Waara]

Attachment: Fix

froodian, does this fix the bug for you?

I've done a fair amount of testing, using a new profile with the huge bookmarks.plist. I verified a lot of different cases in the debugger too, to ensure it all works like I want.

It's very quick now (~2 secs to remove all bookmarks in the 3.1MB bookmarks.plist case), though I've never tried deleting a lot of bookmarks without the patch.

Comment 5

[Håkan Waara]

I have so many changes in this code, it's very hard to do clean diffs, I'm sorry.

You need to add this to BookmarkFolder.mm (and expose the method in BookmarkFolder.h):

-(void) notifyChildrenChanged
{
  [self itemUpdatedNote:kBookmarkItemChildrenChangedMask];
}

Comment 6

[froodian (Ian Leue)]

Attachment: Includes BookmarkFolder changes

This still beachballs for many seconds for me.  It may or may not be an improvement, it's hard to tell.  I only tried running inside of Xcode though, and obviously only on a debug build, so YMMV.

Comment 7

[Håkan Waara]

(In reply to comment #6)
> Created an attachment (id=242793) [edit]
> Includes BookmarkFolder changes
> 
> This still beachballs for many seconds for me.  It may or may not be an
> improvement, it's hard to tell.  I only tried running inside of Xcode though,
> and obviously only on a debug build, so YMMV.

froodian, could you try running a shark session before this patch (with the same build, preferably) and after, and then send both raw sessions to me by email (or upload to an FTP)?  That would be very helpful.

Comment 8

[Håkan Waara]

My suspicion would be that this:

+    unsigned curLevel = [mBookmarksOutlineView levelForItem:doomedBookmark];

Can be expensive, and it might be faster for big cases to remove the whole commonParent-checking thing, and hardcode it to always refresh from the root bookmarks.

(Pass [[BookmarkManager sharedBookmarks] rootBookmarks] as the folder that changed, no matter what, and disable the other code that checks for the common parent level, etc)

Comment 9

[Stuart Morgan]

Comment on attachment 242793 [details] [diff] [review]
Includes BookmarkFolder changes

This is much faster, but it's not right.  If you delete bookmarks from more than one folder and they are on the same level, only one is updated. You need a common parent of all the deleted bookmarks, not just a parent at the lowest level.

Comment 10

[Stuart Morgan]

One option that's be worth considering is using the set of parent folders of all the deleted items; assuming that most deletions of a bunch of selected bookmarks are mostly going to be in a handful of folders, which seems likely to me, that would substantially reduce the number of notifications without adding much complexity or giving an overly broad notification (which would be a perf hit of its own).

Comment 11

[Håkan Waara]

Attachment: Now with even more speed

OK, froodian, please try this.

This is using Smorgan's set idea to avoid having to calculate some sort of mathematically all-encompassing parent of all selected items.

Comment 12

[Håkan Waara]

Comment on attachment 243135 [details] [diff] [review]
Now with even more speed

One unrelated change snuck in (topmost hunk of BMVC.mm), just ignore it as long as you revert the patch after trying it.

Comment 13

[Stuart Morgan]

Comment on attachment 243135 [details] [diff] [review]
Now with even more speed

>+-(void) notifyChildrenChanged;

Use the existing naming scheme.  This is doing the same sort of thing as
  itemAddedNote:atIndex:
  itemRemovedNote:
  itemChangedNote:
I'm not wild about the names either, but having one that's different is worse.


>+    [doomedBookmarkParent deleteChild:doomedBookmark];

doomedBookmarkParent isn't declared, so this isn't going to compile.

Comment 14

[Håkan Waara]

Thanks for catching that Stuart. It should be [doomedBookmark parent].

It's a lot of work for me to create these patches because I have so much other changes near it. So before I get froodian's verification on whether it improves things, please make that change by hand?  Thanks.

Comment 15

[froodian (Ian Leue)]

Attachment: Now with even more speed v2

It seems quite a bit faster, but it still takes far more time than it does to import.  It's certainly a good incremental solution imo.

Comment 16

[Håkan Waara]

(In reply to comment #15)
> Created an attachment (id=243445) [edit]
> Now with even more speed v2
> 
> It seems quite a bit faster, but it still takes far more time than it does to
> import.  It's certainly a good incremental solution imo.

Good. If you're doing a huge operation, then there's no guarantee that we can (easily) make it much faster. The reason importing is perceived is faster might be that it's done on a thread, while this is hogging the main thread.

Comment 17

[Stuart Morgan]

Comment on attachment 243445 [details] [diff] [review]
Now with even more speed v2

>+    [parentsToNotify addObject:[doomedBookmark parent]];
>+    [[doomedBookmark parent] deleteChild:doomedBookmark];

Store [doomedBookmark parent] rather than fetching it twice.

>+  // notify all parents that their children changed.

Change this to
  // notify observers that the parents have changed
since that's what's happening; the parents themselves already know since they did the deletion.

r=me with those changes.

(Ignore my comment before about changing the name of the notify method; I hadn't noticed that those other methods weren't public interface.)

Comment 18

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 243445 [details] [diff] [review]
Now with even more speed v2

yay!

sr=pink

Comment 19

[Håkan Waara]

Done

Comment 20

[Samuel Sidler (old account; do not CC)]

Moving fixed "1.2" bugs to 1.1 where they were really fixed. Filter on CaminoFixed1.1 for bugmail purposes.