1371679 - Use skipDescendantsOnItemRemoval in nsNavHistoryResult

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P1)

Description

[Marco Bonardo [:mak]]

The views basically always care about folders going away, rather than their descendants.
By using skipDescendantsOnItemRemoval, removals handling in bookmark views could become quite faster.

Comment 1

[Mark Banner (:standard8)]

I'll take a look at this.

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1371679 - Use skipDescendantsOnItemRemoval in nsNavHistoryResult to improve performance when deleting bookmark folders.

Review commit: https://reviewboard.mozilla.org/r/163368/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/163368/

Comment 3

[Mark Banner (:standard8)]

Testing with/without this patch and activity stream disabled (in both cases), then when deleting a folder with 613 bookmarks and one empty sub-folder, the time drops from ~487ms to ~408ms

(measured with performance.now() across the PlacesUtils.bookmarks.remove() function on my Mac).

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8892415 [details]
Bug 1371679 - Use skipDescendantsOnItemRemoval in nsNavHistoryResult to improve performance when deleting bookmark folders.

https://reviewboard.mozilla.org/r/163368/#review168760

I assume you did a Try run, I don't know whether this may break some code assumptions!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/409b4345b85a
Use skipDescendantsOnItemRemoval in nsNavHistoryResult to improve performance when deleting bookmark folders. r=mak

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/409b4345b85a

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed this out as requested by Standard8:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5057eb5a60e7f2473af9ddbfafd81b6556d367f6

Comment 8

[Mark Banner (:standard8)]

Thanks, I'll look at this once I'm back.

For others: this caused regressions - bug 1388043 and bug 1389048, so I've gone for the option of backing out and then we can investigate in more detail.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/5057eb5a60e7

Comment 10

[Marco Bonardo [:mak]]

could be due to bug 1316348

Comment 11

[Mark Banner (:standard8)]

Now that bug 1316348 is fixed I'm going to reland this.

I have just been testing it locally, and in the cases of both the sidebar (bug 1388043) and the bookmarks toolbar (bug 1389048) you can now see everything being cleared down, and then displayed with the new content.

Hence I believe the known issues are now fixed here.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3eb1b667b3dd
Use skipDescendantsOnItemRemoval in nsNavHistoryResult to improve performance when deleting bookmark folders. r=mak

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3eb1b667b3dd