1458634 - "forget about this site" in "show all history" blocks the main thread

Status: NEW

(Firefox :: Bookmarks & History, defect, P3)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

STR:
1. Visit many, many pages on a domain (e.g. use dxr extensively)
2. History -> Show All History
3. Select an entry for that domain, right-click, "Forget About This Site"
4. Wait a loooooooong time (the context menu doesn't disappear until the operation completes and nothing else in that window is clickable in the meantime)

Comment 1

[Marco Bonardo [:mak]]

This could be due to the UI view updating on notifications in background, we need new batching notifications to improve that.
Regardless, it could be useful to profile this a bit, there may be low hanging fruits here or other unknown causes.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Here a profile for such a situation. I tried to clear all history for Gmail and ended up with the Firefox UI being blocked for about 80s!

https://share.firefox.dev/3pQUWyc

Note that there is a huge increase of memory usage during that operation. In my case it grows up to 500MB. Also the CPU load is larger than 100% for all that time, which is not happening when I clear all data via History - Clear Recent History.

Marco, do we have any news here?

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Note that when clearing site data via about:preferences the CPU load is not higher than ~40%. So that's a huge difference here.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Also after the browser is responsible again, are are some further delays in event processing: https://share.firefox.dev/3pXH9Gp

This is actually an Activity Stream bug and I filed it as bug 1685140.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

That profile spends lots of time in array.splice, possibly https://searchfox.org/mozilla-central/rev/a0ccd492719b1ad2106f6456549be62a76f45acb/browser/components/places/content/treeView.js#802
But I guess the main reason for that is the missing batching.

Comment 6

[Mark Banner (:standard8)]

I suspect this is a duplicate or largely depends on bug 734643. There's known performance issues with deleting items in the history UI which basically need rewriting of the views. There is some work on notifications going on at the moment that may help (dependencies of bug 1473529).

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Not such a frequent issue that we think should block the power-usage bug.

Comment 8

[Alex Thayer [:alexical] (she/her)]

Marking this as fxperf:p2. If you'd like any help with this, feel free to reach out - it's work that I've been meaning to get back to for a while now, but it looks like given the recent activity on moving other places notifications over, I might just be stepping on toes if I worked on this.

Comment 9

[Marco Bonardo [:mak]]

We should first complete the work on converting notifications, that Daisuke is working on these days, otherwise we'd end up redoing optimizations twice. Of course it would be even nicer to have new views, but it always look like a future target.
Once we have nicer arrays of notifications, we can start by asking the results/views to just fully refresh when the amount of changes is greater than a threshold.

In this case, Forget About This Site, removes all the urls for the given host, if there's thousands of urls, we end up running the single node removal code thousands of times. So yeah, it's pretty much the same batching problem as bug 734643.

Comment 10

[:Gijs (out for now; he/him)]

(In reply to Mark Banner (:standard8) from comment #6)

I suspect this is a duplicate or largely depends on bug 734643. There's known performance issues with deleting items in the history UI which basically need rewriting of the views. There is some work on notifications going on at the moment that may help (dependencies of bug 1473529).

Do we know if this did end up helping?

Comment 11

[Mark Banner (:standard8)]

I think Marco might be in a better position to answer that.

Comment 12

[Marco Bonardo [:mak]]

(In reply to :Gijs (he/him) from comment #10)

Do we know if this did end up helping?

We don't have a measurement to answer easily. The work to move to new notifications is done, but the views still use a synchronous API to requesry when they refresh. So I expect it helped but I also expect it didn't completely solve the problem.

Comment 13

[Mark Banner (:standard8)]

We need a new profile for this happening to evaluate if the fix for bug 734643 helped or not.

We might also want to come up with a "standard" measurement that we can use in future.