Use skipDescendantsOnItemRemoval in nsNavHistoryResult

RESOLVED FIXED in Firefox 57

Status

()

Toolkit
Places
P1
normal
RESOLVED FIXED
9 months ago
6 months ago

People

(Reporter: mak, Assigned: standard8)

Tracking

({perf})

55 Branch
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [fxsearch])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

9 months ago
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.
Priority: P2 → P1
(Assignee)

Comment 1

7 months ago
I'll take a look at this.
Assignee: nobody → standard8
(Assignee)

Updated

7 months ago
Blocks: 1382966
Comment hidden (mozreview-request)
(Assignee)

Comment 3

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

Comment 4

7 months ago
mozreview-review
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!
Attachment #8892415 - Flags: review?(mak77) → review+

Comment 5

7 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/409b4345b85a
Use skipDescendantsOnItemRemoval in nsNavHistoryResult to improve performance when deleting bookmark folders. r=mak
(Reporter)

Updated

7 months ago
Status: NEW → ASSIGNED

Comment 6

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/409b4345b85a
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Updated

7 months ago
Depends on: 1388043
Backed this out as requested by Standard8:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5057eb5a60e7f2473af9ddbfafd81b6556d367f6
Status: RESOLVED → REOPENED
status-firefox57: fixed → ---
Flags: needinfo?(standard8)
Resolution: FIXED → ---
(Assignee)

Comment 8

7 months ago
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.
Flags: needinfo?(standard8)

Comment 9

7 months ago
Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/5057eb5a60e7
Target Milestone: mozilla57 → ---
(Reporter)

Comment 10

6 months ago
could be due to bug 1316348
Depends on: 1316348
(Assignee)

Comment 11

6 months ago
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

6 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3eb1b667b3dd
Use skipDescendantsOnItemRemoval in nsNavHistoryResult to improve performance when deleting bookmark folders. r=mak

Comment 13

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3eb1b667b3dd
Status: REOPENED → RESOLVED
Last Resolved: 7 months ago6 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.