Speedup history removals by simulating a per-statement trigger

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

({regression})

Trunk
mozilla47
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46+ fixed, firefox47+ fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

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.
Created attachment 8722282 [details]
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/
(Assignee)

Updated

3 years ago
Blocks: 1250424
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.
(Assignee)

Updated

3 years ago
Attachment #8722282 - Attachment description: MozReview Request: Bug 1250363 - simpler workaround for bug 871908. r? → MozReview Request: Bug 1250363 - Speed up history removals through a simulated per-statement trigger. r=yoric
Attachment #8722282 - Flags: review?(dteller)
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/
(Assignee)

Updated

3 years ago
Summary: simpler workaround for bug 871908 → Speedup history removals by simulating a per-statement trigger
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".
Attachment #8722282 - Flags: review?(dteller)
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/
Attachment #8722282 - Flags: review?(dteller)
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.
Attachment #8722282 - Flags: review?(dteller) → review+
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 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/37ab9a732b06
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
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
Attachment #8722282 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Blocks: 1206393
Tracking, marking affected for 46.
status-firefox46: --- → affected
tracking-firefox46: --- → +
tracking-firefox47: --- → +
Keywords: regression
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.
Attachment #8722282 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.