Closed Bug 1376149 Opened 7 years ago Closed 7 years ago

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


(Toolkit :: Places, defect, P1)

55 Branch



Tracking Status
firefox-esr52 --- unaffected
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed


(Reporter: tustamido, Assigned: mak)



(Keywords: regression, Whiteboard: [fxsearch])


(1 file)

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)

Certainly it is necessary to have a large places.sqlite to reproduce this bug.
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
Notice that your places.sqlite size is abnormal, did you disable or increase the Places expiration limit?
Flags: needinfo?(tustamido)
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)
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.
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
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
taking to investigate.
Assignee: nobody → mak77
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.
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.
Summary: Regression: poor performance when deleting an item from history → poor performance when deleting an item from history due to favicons removals
Attachment #8921391 - Flags: review?(standard8) → review?(paolo.mozmail)
rebalancing the reviews load.
Comment on attachment 8921391 [details]
Bug 1376149 - Speed up orphan favicons cleanups on history removals.

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+
Attachment #8921391 - Flags: review?(standard8)
Pushed by
Speed up orphan favicons cleanups on history removals. r=Paolo
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.