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

RESOLVED FIXED in Camino1.5

Status

defect
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: froodian, Assigned: hwaara)

Tracking

({fixed1.8.1.1, perf})

unspecified
Camino1.5
PowerPC
macOS

Details

Attachments

(2 attachments, 3 obsolete attachments)

Reporter

Description

13 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

13 years ago
Reporter

Updated

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

Comment 2

13 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.
Yup, batching would fix this.
Assignee

Comment 4

13 years ago
Posted patch Fix (obsolete) — Splinter Review
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

13 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

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

13 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

13 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

13 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

13 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

13 years ago
Posted patch Now with even more speed (obsolete) — Splinter Review
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

13 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

13 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

13 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

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

13 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

13 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

13 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

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

Updated

13 years ago
Keywords: fixed1.8.1.1
Reporter

Updated

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