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.

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

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 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 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 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 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 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 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

Tracking, marking affected for 46.

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.