Currently, Places uses JOURNAL_MODE=WAL and SYNCHRONOUS=FULL. These are acceptable defaults because they will lead to the least amount of data loss in the wild. However, as my measurements have shown (e.g. bug 1271035), this can lead to ridiculous amounts of I/O and slow down test execution and the efficiency of Firefox automation. I reckon most tests don't care about Places at all and Places can be disabled wholesale for these tests. This includes most mochitests. However, disabling Places wholesale for e.g. mochitests is problematic because some tests do rely on Places. Furthermore the more things you disable in tests the more you risk not testing the configuration we ship to users and not exposing bugs. That's a dangerous game to play. I'd like to suggest a compromise: switching the journal_mode and synchronous PRAGMAs to something less safe (read: more prone to data loss) in automation. This will reduce the I/O load on automation while keeping Places running. I don't think we run into issues testing a configuration we don't ship because journal mode and synchronous settings only really come into play when there is e.g. a process or OS crash. Where we test things like corrupted database recovery, yes, we should be testing with WAL and SYNCHRONOUS=FULL. For everything else, testing with the robust SQLite settings doesn't provide any new testing coverage because at that point we're effectively testing SQLite itself, which is shown to have some of the best test coverage of any piece of software ever written (https://www.sqlite.org/testing.html).

This should not be committed as is as it changes the defaults and lowers data guarantees. Review commit: https://reviewboard.mozilla.org/r/51981/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/51981/

Try push is promising. I'm going to code up a proper C++ patch.

We have data showing that the Places SQLite database can consume gigabytes of I/O during Firefox test automation jobs. This is because a number of tests load pages as rapdily as possible, effectively stress testing Places and SQLite. As the SQLite database is committed to, we incur I/O for the WAL journal and when flushing/synchronizing commits. This can add up to a lot of overhead, especially on spinning disks. It is important for Places to run during many tests. But it isn't necessarily important to run with robust I/O guarantees: SQLite itself has tests that ensure different journal and synchronizing modes work as advertised. This commit introduces a preference - "places.database.volatileStorage" - that changes the SQLite journal and synchronization modes to be less robust. We use an in-memory journal so no I/O is incurred for journal writing. We disable synchronization during commit so no expensive file(system) flushing is performed. A preliminary Try run reveals this has the potential to shave hundreds of megabytes of I/O from various jobs. Although this commit stops short of changing the configuration in automation to use the new volatile storage preference. Review commit: https://reviewboard.mozilla.org/r/52005/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/52005/

Comment on attachment 8751395 [details] MozReview Request: Bug 1272025 - Add preference to use volatile storage with Places database; r?mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52005/diff/1-2/

I'm waiting for answers at https://bugzilla.mozilla.org/show_bug.cgi?id=1272060#c11

Comment on attachment 8751395 [details] MozReview Request: Bug 1272025 - Add preference to use volatile storage with Places database; r?mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52005/diff/2-3/

Comment on attachment 8751395 [details] MozReview Request: Bug 1272025 - Add preference to use volatile storage with Places database; r?mak https://reviewboard.mozilla.org/r/52005/#review50296 ::: toolkit/components/places/Database.cpp:57 (Diff revision 3) > +// storage without robust transaction flushing guarantees. This makes > +// SQLite use much less I/O at the cost of losing data when things crash. > +// The pref is only honored if an environment variable is set. The env > +// variable is intentionally named something scary to help prevent someone > +// from thinking it is a useful performance optimization they should enable. > +#define PREF_VOLATILE_STORAGE "places.database.volatileStorage" volatile storage may mean too many things, let's name this: #define PREF_NO_DURABILITY "places.database.disableDurability" ::: toolkit/components/places/Database.cpp:58 (Diff revision 3) > +// SQLite use much less I/O at the cost of losing data when things crash. > +// The pref is only honored if an environment variable is set. The env > +// variable is intentionally named something scary to help prevent someone > +// from thinking it is a useful performance optimization they should enable. > +#define PREF_VOLATILE_STORAGE "places.database.volatileStorage" > +#define ENV_VOLATILE_STORAGE "ALLOW_PLACES_DATABASE_TO_LOSE_DATA_AND_BECOME_CORRUPT" #define ENV_ALLOW_CORRUPTION ::: toolkit/components/places/Database.cpp:639 (Diff revision 3) > nsresult rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( > MOZ_STORAGE_UNIQUIFY_QUERY_STR "PRAGMA temp_store = MEMORY")); > NS_ENSURE_SUCCESS(rv, rv); > > + // By default use the WAL as that is the most robust. > + if (!Preferences::GetBool(PREF_VOLATILE_STORAGE, false)) { this looks wrong. If the env is not set, we should completely ignore the pref. should be if (PR_GetEnv(ENV_ALLOW_CORRUPTION) && Preferences::GetBool(PREF_NO_DURABILITY, false)) { // Use an in-memory journal becuse bla bla... ...sync = off } else { ..normal path }

Comment on attachment 8751395 [details] MozReview Request: Bug 1272025 - Add preference to use volatile storage with Places database; r?mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52005/diff/3-4/

Comment on attachment 8751395 [details] MozReview Request: Bug 1272025 - Add preference to use volatile storage with Places database; r?mak https://reviewboard.mozilla.org/r/52005/#review50928