459491 - nsPlacesDBFlush could use async queries instead of background thread

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak]]

Using async queries we can avoid the time needed to create an additional background thread

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch

my first idea to integrate all into a single trigger does not work always, that's because delete trigger on moz_historyvisits does not sync places that have been added as bookmarks during a batch (they don't have a visit and we have not synced them while in batch), so i've added a syncMozHistory method that is called on the timer, while on quit application and on endUpdateBatch i must use 2 statements in syncAll (the first will sync visits and places with visits, the second syncs remaining places).

So on timer we still have only 1 fsync, while we will have 2 fsyncs in these 2 cases. Any idea about this?

asking first-pass. Shawn what you think we should do if in handleCompletion we get an error reason?

Comment 2

[Marco Bonardo [:mak]]

Attachment: patch

wrong version, this is correct

Comment 3

[Shawn Wilsher :sdwilsh]

(In reply to comment #1)
> So on timer we still have only 1 fsync, while we will have 2 fsyncs in these 2
> cases. Any idea about this?
bug 458811 landed, so that solves our issue, right?

> asking first-pass. Shawn what you think we should do if in handleCompletion we
> get an error reason?
We should log something to the error console (likely when we get handleError though, as that /might/ be able to give us more information).

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 342705 [details] [diff] [review]
patch

It'd be more helpful to have a patch generated with at least 8 lines of context.  Some of this is hard to follow.

>diff --git a/toolkit/components/places/src/nsPlacesDBFlush.js b/toolkit/components/places/src/nsPlacesDBFlush.js

>   // Get our sync interval
>-  this._prefs = Cc["@mozilla.org/preferences-service;1"].
>-                getService(Ci.nsIPrefService).
>-                getBranch("places.");
>   try {
>     // We want to silently fail if the preference does not exist, and use a
>     // default to fallback to.

>   // Register observers
>-  this._bh.addObserver(this, false);
>-
>-  this._prefs.QueryInterface(Ci.nsIPrefBranch2).addObserver("", this, false);
>-
>-  let os = Cc["@mozilla.org/observer-service;1"].
>-           getService(Ci.nsIObserverService);
>-  os.addObserver(this, kPlacesBackgroundShutdown, false);
>+  this._bs.addObserver(this, false);
>+  this._os.addObserver(this, kQuitApplication, false);
>+  this._prefs.QueryInterface(Ci.nsIPrefBranch2)
>+             .addObserver("", this, false);
So, since we use this._bs, this._os, and this._prefs, I don't see how using smart getters actually helps us.  Technically it probably hurts us since the JS engine has to parse those at startup, then execute them.  We should just probably assign if we need to use it when creating the component.

>diff --git a/toolkit/components/places/src/nsPlacesTriggers.h b/toolkit/components/places/src/nsPlacesTriggers.h

>-#define CREATE_TEMP_SYNC_TRIGGER_BASE(__table) NS_LITERAL_CSTRING( \
>-  "CREATE TEMPORARY TRIGGER " __table "_beforedelete_trigger " \
>-  "BEFORE DELETE ON " __table "_temp FOR EACH ROW " \
>+
>+#define CREATE_MOZ_PLACES_SYNC_TRIGGER NS_LITERAL_CSTRING( \
>+  "CREATE TEMPORARY TRIGGER moz_places_beforedelete_trigger " \
>+  "BEFORE DELETE ON moz_places_temp FOR EACH ROW " \
>   "BEGIN " \
>-    "INSERT OR REPLACE INTO " __table " " \
>-    "SELECT * FROM " __table "_temp " \
>+    "INSERT OR REPLACE INTO moz_places " \
>+    "SELECT * FROM moz_places_temp " \
>     "WHERE id = OLD.id;" \
>   "END" \
> )
>-#define CREATE_MOZ_PLACES_SYNC_TRIGGER \
>-  CREATE_TEMP_SYNC_TRIGGER_BASE("moz_places")
>-#define CREATE_MOZ_HISTORYVISITS_SYNC_TRIGGER \
>-  CREATE_TEMP_SYNC_TRIGGER_BASE("moz_historyvisits")
>+
>+/**
>+ * This trigger is similar to the above one, but before moving data we ensure
>+ * that the place has been already moved to the permanent table.
>+ */
>+#define CREATE_MOZ_HISTORYVISITS_SYNC_TRIGGER NS_LITERAL_CSTRING( \
>+  "CREATE TEMPORARY TRIGGER moz_historyvisits_beforedelete_trigger " \
>+  "BEFORE DELETE ON moz_historyvisits_temp FOR EACH ROW " \
>+  "BEGIN " \
>+    "DELETE FROM moz_places_temp WHERE id = OLD.place_id; " \
>+    "INSERT OR REPLACE INTO moz_historyvisits " \
>+    "SELECT * FROM moz_historyvisits_temp " \
>+    "WHERE id = OLD.id; " \
>+  "END" \
>+)
I'd like to keep this the way it was, and just use the multiple statement api that was added recently.  It means there is less of a chance that we break something in one query when they share the same code.

>diff --git a/toolkit/components/places/tests/background/head_background.js b/toolkit/components/places/tests/background/head_background.js
we should copy that dump_table function to the other head_ files before it goes away.  I've found it to be incredibly useful.

>diff --git a/toolkit/components/places/tests/sync/head_sync.js b/toolkit/components/places/tests/sync/head_sync.js
can you please use hg cp so that all this stuff you copied from another directory doesn't show up in the diffs (and then we don't have to review it)

>+function finish_test()
>+{
>+  // This next bit needs to run on the main thread
>+  let tm = Cc["@mozilla.org/thread-manager;1"].
>+           getService(Ci.nsIThreadManager);
>+  tm.mainThread.dispatch({
>+    run: function()
>+    {
>+      // xpcshell doesn't dispatch shutdown-application
>+      let os = Cc["@mozilla.org/observer-service;1"].
>+               getService(Ci.nsIObserverService);
>+      os.notifyObservers(null, "quit-application", null);
>+      do_test_finished();
>+    }
>+  }, Ci.nsIEventTarget.DISPATCH_NORMAL);
>+}
you probably don't need to do this quite like this anymore (we should already be on the main thread)

>+function new_test_bookmark_uri_event(aBookmarkId, aExpectedURI, aExpected, aFinish)
weird indentation in this function now

>+function new_test_visit_uri_event(aVisitId, aExpectedURI, aExpected, aFinish)
ditto

>diff --git a/toolkit/components/places/tests/sync/test_database_sync_after_addBookmark.js b/toolkit/components/places/tests/sync/test_database_sync_after_addBookmark.js
why don't you hg move these files so the diffs are smaller and represent what you actually changed please

Comment 5

[Marco Bonardo [:mak]]

(In reply to comment #4)
> >diff --git a/toolkit/components/places/tests/background/head_background.js b/toolkit/components/places/tests/background/head_background.js
> we should copy that dump_table function to the other head_ files before it goes
> away.  I've found it to be incredibly useful.

it will survive in head_sync.js, we can move to other headers later if it's needed

Comment 6

[Marco Bonardo [:mak]]

Attachment: all of the above

for the error to the console i'm unsure about what to print... we probablly want a translatable error printed and getting a stringbundle, so i would leave that for a followup, unless you have an alternative idea.

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 343207 [details] [diff] [review]
all of the above

>diff --git a/toolkit/components/places/src/nsPlacesDBFlush.js b/toolkit/components/places/src/nsPlacesDBFlush.js
> nsPlacesDBFlush.prototype = {
>+  _syncInterval: kDefaultSyncInterval,
>+
nit: can you dump this under the section labeled with "//// nsPlacesDBFlush" please?

>+  handleCompletion: function(aReason)
name the function please

>+  {
>+    switch (aReason) {
>+      case Ci.mozIStorageStatementCallback.REASON_ERROR:
>+      //XXX print an error to the console (manage handleError too)
>+      case Ci.mozIStorageStatementCallback.REASON_CANCELED:
>+      //XXX print an error to the console
>+      case Ci.mozIStorageStatementCallback.REASON_FINISHED:
>+        // Dispatch a notification that sync has finished.
>+        this._os.notifyObservers(null, kSyncFinished, null);
I think we should just check to make sure reason is FINISHED, and then dispatch the observer.  We don't ever cancel, and if we want to log an error, we should print something to the console and use handleError.  Simpler code FTW.

>   //////////////////////////////////////////////////////////////////////////////
>   //// nsPlacesDBFlush
> 
>   /**
>-   * Dispatches an event to the background thread to run _doSyncMozX("places").
>+   * Execute async statements to sync temporary places table.
>+   * All of this is done in a transaction that is rolled back upon failure at
>+   * any point.
nit: second sentance isn't really needed - part of the executeAsync API.  Also, please include and @params for aTableNames

>     // No need to do extra work if we are in batch mode
>     if (this._inBatchMode)
>       return;
>+    let statements = [];
>+    for (let i = 0; i < aTableNames.length; i++)
>+      statements.push(this._getSyncTableStatement(aTableNames[i]));
nit: newline after return please

> 
>   /**
>-   * Dispatches an event to the background thread to sync all temporary tables.
>+   * Generate the statement to synchronizes the moz_{aName} and moz_{aName}_temp
>+   * by copying all the data from the temporary table into the permanent one.
>+   * It then deletes the data in the temporary table.
probably want to indicate that a bunch of this is done via triggers (found in nsPlacesTriggers.h)

>+  _getSyncTableStatement: function DBFlush_getSyncTableStatement(aTableName)
>   {
>+    // Delete all the data in the temp table.
>+    // We have triggers setup that ensure that the data is transfered over
nit: transferred

>+try {
>+  var bs = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
>+           getService(Ci.nsINavBookmarksService);
>+  var os = Cc["@mozilla.org/observer-service;1"].
>+           getService(Ci.nsIObserverService);
>+  var prefs = Cc["@mozilla.org/preferences-service;1"].
>+              getService(Ci.nsIPrefService).
>+              getBranch("places.");
>+}
>+catch(ex) {
>+  do_throw("Could not get services\n");
>+}
I never really understood why places tests do this.  If we can't get the services, they'll throw anyway, giving us a more accurate line number.  Please don't do this.

r=sdwilsh with the above fixed and what we discussed over irc

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch

addressed review comments

Comment 9

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/9d61859ba839

Comment 10

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv