Closed Bug 616309 Opened 9 years ago Closed 9 years ago

Use normal synchronous mode with a small WAL. Temp tables revenge!

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b9
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mak, Assigned: mak)

References

Details

(Keywords: perf, Whiteboard: [fixed-in-places])

Attachments

(2 files, 2 obsolete files)

The problem is that without temp tables we have more fsyncs, not a lot more because we have made our best to reduce them, and those are also on the background thread. But we still fight the lovely Linux filesystem.

The idea is to use WAL to reproduce the temp tables behavior, i.e. get temp tables advantages without having their disadvantages.

In WAL mode with synchronous full any single transaction is committed to the WAL with a fsync, then on specified times (size constraints really) the journal is committed to the main database with a fsync.
This differs from before since writing to temp tables was avoiding the fsyncs (not always though, for bookmarks we were still forcing flushes).

Changing our synchronous mode to normal (the Storage default), will not fsync anymore commits to the WAL, but checkpoints to the main database will still be fsynced. This means we can have almost (disk is still slower than memory) the same speed as temp tables, since we don't fsync every single transaction or write. Writes will be really cheap.

Talking about data safety, the main database cannot even corrupt, because the wal is checkpointed to it with fsyncs (we could also evaluate to enable fullfsyncs on Mac). The worst case scenario is that after a crash the full WAL gets lost (rather than just the last transaction), and with it we lose all the last transactions. This is partly similar to a crash with not-yet-flushed temp tables, probably better because we lose full transactions rather then partial transactions with partial data.
Ways to mitigate data safety issues are:
- use a small enough WAL, so that eventual size of lost data is small
- force checkpoints at strategic points
Assignee: nobody → mak77
Status: NEW → ASSIGNED
blocking2.0: --- → betaN+
Attached patch part 1: normal synchronous wal (obsolete) — Splinter Review
I tried to skip the methods that are not that interesting from a dataloss point of view, like insertSeparator or setting a lastModified
Attachment #494888 - Flags: review?(sdwilsh)
Attachment #494865 - Flags: review?(sdwilsh)
Comment on attachment 494865 [details] [diff] [review]
part 1: normal synchronous wal

>+++ b/toolkit/components/places/src/nsNavHistory.cpp
>+#define DATABASE_MAX_WAL_SIZE_IN_BYTES 524288
I would prefer this to be DATABASE_MAX_WAL_SIZE_IN_KILOBYTES as that is easier to reason with than bytes.  Just do the math at first use please.

r=sdwilsh with that fixed.
Attachment #494865 - Flags: review?(sdwilsh) → review+
looks like I forgot to save Helpers.h, that forceWALCheckpoint should be uppercase. fixed locally.
Comment on attachment 494888 [details] [diff] [review]
part 2: force checkpoints in bookmarks

>+++ b/toolkit/components/places/src/Helpers.cpp
>+void
>+ForceWALCheckpoint(mozIStorageConnection* aDBConn)
>+{
>+  nsCOMPtr<mozIStorageAsyncStatement> stmt;
>+  aDBConn->CreateAsyncStatement(NS_LITERAL_CSTRING(
nit: (void)

>+  NS_ASSERTION(stmt, "Should be possible to force a WAL checkpoint");
>+  if (stmt) {
This should only fail in out of memory situations, which would end up aborting anyway.  Lose the assertion, and the null check.

>+++ b/toolkit/components/places/src/Helpers.h
>+/**
>+ * Forces a WAL checkpoint.
>+ * This will cause all transactions stored in the journal file to be committed
nit: lose the newline between these two lines

>+ * @param aDBConn
>+ *        Connection to the database.
>+ * @note The checkpoint will force a fsync.
nit: "force an fsync/Flush".  Let's start to include the windows method name too ;)

>+void
>+forceWALCheckpoint(mozIStorageConnection* aDBConn);
nit: in .h files, we keep the return type on the same line.

r=sdwilsh
Attachment #494888 - Flags: review?(sdwilsh) → review+
Attachment #494865 - Attachment is obsolete: true
Attachment #494888 - Attachment is obsolete: true
part 1: http://hg.mozilla.org/projects/places/rev/9d57c1a44480
part 2: http://hg.mozilla.org/projects/places/rev/86b924f61b01
Flags: in-testsuite-
Whiteboard: [fixed-in-places]
Keywords: perf
Blocks: 585704
Depends on: 616425
http://hg.mozilla.org/mozilla-central/rev/9d57c1a44480
http://hg.mozilla.org/mozilla-central/rev/86b924f61b01
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b9
You need to log in before you can comment on or make changes to this bug.