1250363 - Speedup history removals by simulating a per-statement trigger

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

bug 871908 causes hangs when we try to clear history.
To properly fix it we need some deep changes to how we store hosts, and likely the complete removal of triggers, that look involved into the problem.
It would also be nice to get some hints from the Sqlite team.

In the meanwhile, we need a simpler fix that we can uplift for bug 1248489.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: MozReview Request: Bug 1250363 - Speed up history removals through a simulated per-statement trigger. r=yoric

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

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

On my default profile, with 110k places, with the patch I can clear history in about 35s (note though this is an i7 with an SSD, so we may have to double the value to get a realistic number). We still generate a 4GB wal journal but at least it's manageable.
Without the patch the browser is still hanging, after 5 minutes that I launched the DELETE query, and doesn't seem to proceed. From my last measurement we were in the tens of minutes...

We'll look for a definitive solution for the large wal file in bug 871908, maybe with the help of the Sqlite team. I don't think here we can go further with a patch that we can uplift safely.

On an history that is per session (clear on shutdown), thus much smaller than mine, this should do a great job.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8722282 [details]
MozReview Request: Bug 1250363 - Speed up history removals through a simulated per-statement trigger. r=yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35963/diff/1-2/

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8722282 [details]
MozReview Request: Bug 1250363 - Speed up history removals through a simulated per-statement trigger. r=yoric

https://reviewboard.mozilla.org/r/35963/#review32861

I believe that I understand how the patch works and why it doesn't break anything (although I'll take a second look later, just to be sure), but that's not sufficient for me to understand why it makes things faster.

Could you document this in the commit message?

::: toolkit/components/places/nsPlacesTables.h:147
(Diff revision 2)
> +#define CREATE_UPDATEHOSTS_TEMP NS_LITERAL_CSTRING( \

Can you document what the table contains?

I realize that you document essentially that in `nsPlacesTriggers.h`, but it would be a good idea to have this also close to the definition of the table itself.

::: toolkit/components/places/nsPlacesTriggers.h:121
(Diff revision 2)
> +// Regardless, this is still better than hanging the browser for a long time.

Nit: "a long time" => "several minutes on a fast machine".

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8722282 [details]
MozReview Request: Bug 1250363 - Speed up history removals through a simulated per-statement trigger. r=yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35963/diff/2-3/

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8722282 [details]
MozReview Request: Bug 1250363 - Speed up history removals through a simulated per-statement trigger. r=yoric

https://reviewboard.mozilla.org/r/35963/#review32883

Looks good to me. Here's hoping that this solves most of our issues.

::: toolkit/components/places/History.cpp:1901
(Diff revision 3)
> +      nsAutoCString query("DELETE FROM moz_updatehosts_temp");

Maybe a small comment to re-explain that the first request adds the hosts to the temp table and the second one empties it, etc.

::: toolkit/components/places/History.jsm:507
(Diff revision 3)
> +  yield db.execute(`DELETE FROM moz_updatehosts_temp`);

Maybe a small comment to re-explain that the first request adds the hosts to the temp table and the second one empties it, etc.

::: toolkit/components/places/nsNavHistory.cpp:2459
(Diff revision 3)
> +    NS_LITERAL_CSTRING("DELETE FROM moz_updatehosts_temp")

Maybe a small comment to re-explain that the first request adds the hosts to the temp table and the second one empties it, etc.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8722282 [details]
MozReview Request: Bug 1250363 - Speed up history removals through a simulated per-statement trigger. r=yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35963/diff/3-4/

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/37ab9a732b06

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/37ab9a732b06

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8722282 [details]
MozReview Request: Bug 1250363 - Speed up history removals through a simulated per-statement trigger. r=yoric

Approval Request Comment
[Feature/regressing bug #]: very old regression
[User impact if declined]: We are crashing on shutdown due to the sanitizer taking too much time, and hanging the UI when sanitizing on startup. This patch makes history removals MUCH faster.
[Describe test coverage new/current, TreeHerder]: lots of Places tests touch these codepaths. Currently in Nightly
[Risks and why]: no known risks so far
[String/UUID change made/needed]: none

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking, marking affected for 46.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8722282 [details]
MozReview Request: Bug 1250363 - Speed up history removals through a simulated per-statement trigger. r=yoric

Workaround for shutdown crash, please uplift to aurora.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d755e1b8c8f0