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

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Bookmarks
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: froodian (Ian Leue), Assigned: Håkan Waara)

Tracking

({fixed1.8.1.1, perf})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1.1, perf

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

11 years ago
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
(Reporter)

Comment 1

11 years ago
Created attachment 242609 [details]
Sample of Camino Beachballing
(Reporter)

Updated

11 years ago
Target Milestone: --- → Camino1.2

Comment 2

11 years ago
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

11 years ago
Yup, batching would fix this.
(Assignee)

Comment 4

11 years ago
Created attachment 242788 [details] [diff] [review]
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.
Assignee: nobody → hwaara
Status: NEW → ASSIGNED
Attachment #242788 - Flags: review?(stuart.morgan)
(Assignee)

Comment 5

11 years ago
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];
}
(Reporter)

Comment 6

11 years ago
Created attachment 242793 [details] [diff] [review]
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.
Attachment #242788 - Attachment is obsolete: true
Attachment #242793 - Flags: review?(stuart.morgan)
Attachment #242788 - Flags: review?(stuart.morgan)
(Assignee)

Comment 7

11 years ago
(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.
(Assignee)

Comment 8

11 years ago
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

11 years ago
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.
Attachment #242793 - Flags: review?(stuart.morgan) → review-

Comment 10

11 years ago
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).
(Assignee)

Comment 11

11 years ago
Created attachment 243135 [details] [diff] [review]
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.
Attachment #242793 - Attachment is obsolete: true
(Assignee)

Comment 12

11 years ago
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

11 years ago
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.
(Assignee)

Comment 14

11 years ago
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.
(Reporter)

Comment 15

11 years ago
Created attachment 243445 [details] [diff] [review]
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.
Attachment #243135 - Attachment is obsolete: true
Attachment #243445 - Flags: review?(stuart.morgan)
(Assignee)

Comment 16

11 years ago
(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

11 years ago
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.)
Attachment #243445 - Flags: review?(stuart.morgan) → review+
(Assignee)

Updated

11 years ago
Attachment #243445 - Flags: superreview?(mikepinkerton)
Comment on attachment 243445 [details] [diff] [review]
Now with even more speed v2

yay!

sr=pink
Attachment #243445 - Flags: superreview?(mikepinkerton) → superreview+
Whiteboard: [needs checkin hwaara?]
(Assignee)

Comment 19

11 years ago
Done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [needs checkin hwaara?]
(Reporter)

Updated

11 years ago
Keywords: fixed1.8.1.1
(Reporter)

Updated

11 years ago
Keywords: fixed1.8.1
Moving fixed "1.2" bugs to 1.1 where they were really fixed. Filter on CaminoFixed1.1 for bugmail purposes.
Target Milestone: Camino1.2 → Camino1.1
You need to log in before you can comment on or make changes to this bug.