poor performance when deleting an item from history due to favicons removals

RESOLVED FIXED in Firefox 58

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: tustamido, Assigned: mak)

Tracking

(Blocks 1 bug, {regression})

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

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 wontfix, firefox57 wontfix, firefox58 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
Since 55.0b1 I noticed a huge performance drop when deleting an item from History.

I use Imagus extension, which allows to record/delete an image URL in History by pressing 'h' on mouseover the image link on page.

Since 55.0b1, when I press 'h' to remove an item from history my Firefox freezes for more than a second (105MB places.sqlite).

Using the same profile on Fx 54 there is no freezing while performing the same action.

e10s always disabled.

The same behavior could be seen by manually deleting an item from History (Places Organizer): freezes on 55.0b1, no issue on 54.0. But it seems a little less noticeable than when deleting from the page using 'h' Imagus feature, outside Places Organizes.

mozregression (autoland)
Last good buildid: 20170412073408 (changeset b01f44c8)
First bad buildid: 20170412080008 (changeset d80c5176)

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=bc086e9044e6537d18d385f417248cdb4c14c3af&tochange=55e46fac1625903640f99cf32cafac545dcb2fdb

Certainly it is necessary to have a large places.sqlite to reproduce this bug.
(Assignee)

Comment 1

2 years ago
This is something to investigate and profile on our side. Mostly the second part, that is deleting an item from history.

Do you see the same freeze if you temporarily disable the extension (or Safe Mode https://support.mozilla.org/kb/troubleshoot-firefox-issues-using-safe-mode)?
Notice that your places.sqlite size is abnormal, did you disable or increase the Places expiration limit?
Flags: needinfo?(tustamido)
(Reporter)

Comment 2

2 years ago
Still reproducible in safe mode.

Yes, I use your Expire history by days. Tried Places Maintenance too.

Although reproducible in safe mode, I can't reproduce it in a new profile (replacing the empty places.sqlite by the large one). There must be something special in my profile, but even copying my prefs.js to the new profile I still can't reproduce the issue.

But when I open my *damaged* profile on Fx 54- I don't see the freeze, so seems to be related to some change in Fx 55 (probably between changesets b01f44c8 and d80c5176 according to tests with mozregression).
Flags: needinfo?(tustamido)
(Reporter)

Comment 3

2 years ago
Seems to be my 35MB favicons.sqlite. Copied it to the new profile and then I was able to reproduce the issue. Deleted the file and the issue was gone. Probably related to bug 977177, code was pushed within the range I posted above.
(Assignee)

Comment 4

2 years ago
Thanks, we indeed added code to expire icons along with pages, and that may be related.
We didn't spend lots of time profiling icons removal in a large places.sqlite case, since the defult expiration keep things "good enough".

We should check what's up.
Blocks: 977177
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Priority: -- → P1
Whiteboard: [fxsearch]
Not sure if this will make 57, but we still would like to fix it soon.
Keywords: stale-bug
Blocks: 1397267
(Assignee)

Comment 7

2 years ago
taking to investigate.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Discussed this with Marco and it sounds like we don't need to track this bug for 57/58. There have been a few perf improvements in this area already, but he is still investigating if there are other bottlenecks here, too.
(Assignee)

Comment 9

2 years ago
Considering comment 3, I can assume the time is spent in History.jsm::cleanupPages, where we remove orphan icons. This is done for privacy reasons since it's related to history removals.
On my system the 2 queries to clear icons take 400ms, but I'm on a far quick system, so that's likely to be multiplied by 4 times (or even more on some really cheap devices).
The big difference compared to before, is that we used to do just the history removals synchronously and then delegate the work to async expiration. Though, that creates far more complex and spread around code, and it's not as privacy-safe, since expiration can be interrupted and may not complete before shutdown. That ended up creating various bug reports about private stuff staying in the profile after shutdown.

We could use a trigger to remove icons when relations go, but I don't think it would be more efficient, indeed while currently we pay a fixed price regardless of the removal size, using such a trigger would be O(n), at a minimum, having to check for each removal if the icon is still used or not. After a few pages, it may be far more expensive.
I also thought of using idleDispatchToMainThread, but it would be complicate, because we should also ensure it runs before shutdown. I'd keep this as a second option for the future, if we see the need.

For now I can tweak the 2 queries to reduce that time to about 100ms.
I'm prone to try this solution, collect feedback and iterate in further bugs where necessary.

That said, I cannot explain the freeze, the UI may take a little bit to update, but it should not hang Firefox, unless there's some synchronous query that is triggered by it and waits on the async operation. That's a possibility, but it's hard to prevent without rewriting the views. I'm assuming the hang in reality is just noticing the UI takes some time before removing the entry. Our views in general work on notifications from the model, rather than doing the operation locally and assuming the model will update in background. This was an initial design choice that is expensive to fix now.
(Assignee)

Updated

2 years ago
Summary: Regression: poor performance when deleting an item from history → poor performance when deleting an item from history due to favicons removals
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8921391 - Flags: review?(standard8) → review?(paolo.mozmail)
(Assignee)

Comment 12

2 years ago
rebalancing the reviews load.

Comment 13

2 years ago
mozreview-review
Comment on attachment 8921391 [details]
Bug 1376149 - Speed up orphan favicons cleanups on history removals.

https://reviewboard.mozilla.org/r/192426/#review198530

Looks good to me.

::: toolkit/components/places/History.jsm:865
(Diff revision 2)
> -    await db.executeCached(`DELETE FROM moz_pages_w_icons
> +  await db.executeCached(`DELETE FROM moz_pages_w_icons
> -                            WHERE page_url_hash NOT IN (SELECT url_hash FROM moz_places)`);
> +    WHERE page_url_hash IN (${sqlList(hashesToRemove)})`);

nit: might consider re-indenting either this one or the statements below
Attachment #8921391 - Flags: review?(paolo.mozmail) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8921391 - Flags: review?(standard8)

Comment 15

2 years ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/4b94d394f4ef
Speed up orphan favicons cleanups on history removals. r=Paolo
https://hg.mozilla.org/mozilla-central/rev/4b94d394f4ef
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.