Bug 1364488 - Allow fetch to use a concurrent connection. r=standard8
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 17 May 2017 15:51:02 +0200
changeset 407100 2e57846b3e5708b8e0e2924d9f85b59e5f461ec2
parent 407099 fd205946fab5432a82361a521d0757c2d79f6458
child 407101 9d4c6f246ae6ede00341da857d25563112ba76d9
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersstandard8
bugs1364488
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1364488 - Allow fetch to use a concurrent connection. r=standard8 MozReview-Commit-ID: 4L4PFtXsjsy
browser/base/content/browser-places.js
toolkit/components/places/Bookmarks.jsm
toolkit/components/places/tests/bookmarks/test_bookmarks_fetch.js
toolkit/modules/Sqlite.jsm
--- a/browser/base/content/browser-places.js
+++ b/browser/base/content/browser-places.js
@@ -1669,17 +1669,17 @@ var BookmarkingUI = {
     this._uri = gBrowser.currentURI;
     this._itemGuids.clear();
     let guids = new Set();
 
     // those objects are use to check if we are in the current iteration before
     // returning any result.
     let pendingUpdate = this._pendingUpdate = {};
 
-    PlacesUtils.bookmarks.fetch({url: this._uri}, b => guids.add(b.guid))
+    PlacesUtils.bookmarks.fetch({url: this._uri}, b => guids.add(b.guid), { concurrent: true })
       .catch(Components.utils.reportError)
       .then(() => {
          if (pendingUpdate != this._pendingUpdate) {
            return;
          }
 
          // It's possible that onItemAdded gets called before the async statement
          // calls back.  For such an edge case, retain all unique entries from the
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -756,28 +756,33 @@ var Bookmarks = Object.freeze({
    *      To retrieve ALL of the bookmarks for that URL, you must pass in an
    *      onResult callback, that will be invoked once for each found bookmark.
    *
    * @param guidOrInfo
    *        The globally unique identifier of the item to fetch, or an
    *        object representing it, as defined above.
    * @param onResult [optional]
    *        Callback invoked for each found bookmark.
+   * @param options [optional]
+   *        an optional object whose properties describe options for the fetch:
+   *         - concurrent: fetches concurrently to any writes, returning results
+   *                       faster. On the negative side, it may return stale
+   *                       information missing the currently ongoing write.
    *
    * @return {Promise} resolved when the fetch is complete.
    * @resolves to an object representing the found item, as described above, or
    *           an array of such objects.  if no item is found, the returned
    *           promise is resolved to null.
    * @rejects if an error happens while fetching.
    * @throws if the arguments are invalid.
    *
    * @note Any unknown property in the info object is ignored.  Known properties
    *       may be overwritten.
    */
-  fetch(guidOrInfo, onResult = null) {
+  fetch(guidOrInfo, onResult = null, options = {}) {
     if (onResult && typeof onResult != "function")
       throw new Error("onResult callback must be a valid function");
     let info = guidOrInfo;
     if (!info)
       throw new Error("Input should be a valid object");
     if (typeof(info) != "object") {
       info = { guid: guidOrInfo };
     } else if (Object.keys(info).length == 1) {
@@ -807,21 +812,21 @@ var Bookmarks = Object.freeze({
 
     // Even if we ignore any other unneeded property, we still validate any
     // known property to reduce likelihood of hidden bugs.
     let fetchInfo = validateBookmarkObject(info, behavior);
 
     return (async function() {
       let results;
       if (fetchInfo.hasOwnProperty("url"))
-        results = await fetchBookmarksByURL(fetchInfo);
+        results = await fetchBookmarksByURL(fetchInfo, options && options.concurrent);
       else if (fetchInfo.hasOwnProperty("guid"))
-        results = await fetchBookmark(fetchInfo);
+        results = await fetchBookmark(fetchInfo, options && options.concurrent);
       else if (fetchInfo.hasOwnProperty("parentGuid") && fetchInfo.hasOwnProperty("index"))
-        results = await fetchBookmarkByPosition(fetchInfo);
+        results = await fetchBookmarkByPosition(fetchInfo, options && options.concurrent);
 
       if (!results)
         return null;
 
       if (!Array.isArray(results))
         results = [results];
       // Remove non-enumerable properties.
       results = results.map(r => Object.assign({}, r));
@@ -1381,82 +1386,95 @@ async function queryBookmarks(info) {
       return rowsToItemsArray(rows);
     }
   );
 }
 
 
 // Fetch implementation.
 
-function fetchBookmark(info) {
-  return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: fetchBookmark",
-    async function(db) {
-
+async function fetchBookmark(info, concurrent) {
+  let query = async function(db) {
     let rows = await db.executeCached(
       `SELECT b.guid, IFNULL(p.guid, "") AS parentGuid, b.position AS 'index',
               b.dateAdded, b.lastModified, b.type, b.title, h.url AS url,
               b.id AS _id, b.parent AS _parentId,
               (SELECT count(*) FROM moz_bookmarks WHERE parent = b.id) AS _childCount,
               p.parent AS _grandParentId, b.syncStatus AS _syncStatus
        FROM moz_bookmarks b
        LEFT JOIN moz_bookmarks p ON p.id = b.parent
        LEFT JOIN moz_places h ON h.id = b.fk
        WHERE b.guid = :guid
       `, { guid: info.guid });
 
     return rows.length ? rowsToItemsArray(rows)[0] : null;
-  });
+  };
+  if (concurrent) {
+    let db = await PlacesUtils.promiseDBConnection();
+    return query(db);
+  }
+  return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: fetchBookmark",
+                                           query);
 }
 
-function fetchBookmarkByPosition(info) {
-  return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: fetchBookmarkByPosition",
-    async function(db) {
+async function fetchBookmarkByPosition(info, concurrent) {
+  let query = async function(db) {
     let index = info.index == Bookmarks.DEFAULT_INDEX ? null : info.index;
-
     let rows = await db.executeCached(
       `SELECT b.guid, IFNULL(p.guid, "") AS parentGuid, b.position AS 'index',
               b.dateAdded, b.lastModified, b.type, b.title, h.url AS url,
               b.id AS _id, b.parent AS _parentId,
               (SELECT count(*) FROM moz_bookmarks WHERE parent = b.id) AS _childCount,
               p.parent AS _grandParentId, b.syncStatus AS _syncStatus
        FROM moz_bookmarks b
        LEFT JOIN moz_bookmarks p ON p.id = b.parent
        LEFT JOIN moz_places h ON h.id = b.fk
        WHERE p.guid = :parentGuid
        AND b.position = IFNULL(:index, (SELECT count(*) - 1
                                         FROM moz_bookmarks
                                         WHERE parent = p.id))
       `, { parentGuid: info.parentGuid, index });
 
     return rows.length ? rowsToItemsArray(rows)[0] : null;
-  });
+  };
+  if (concurrent) {
+    let db = await PlacesUtils.promiseDBConnection();
+    return query(db);
+  }
+  return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: fetchBookmarkByPosition",
+                                           query);
 }
 
-function fetchBookmarksByURL(info) {
+async function fetchBookmarksByURL(info, concurrent) {
+  let query = async function(db) {
+    let tagsFolderId = await promiseTagsFolderId();
+    let rows = await db.executeCached(
+      `/* do not warn (bug no): not worth to add an index */
+      SELECT b.guid, IFNULL(p.guid, "") AS parentGuid, b.position AS 'index',
+              b.dateAdded, b.lastModified, b.type, b.title, h.url AS url,
+              b.id AS _id, b.parent AS _parentId,
+              NULL AS _childCount, /* Unused for now */
+              p.parent AS _grandParentId, b.syncStatus AS _syncStatus
+      FROM moz_bookmarks b
+      JOIN moz_bookmarks p ON p.id = b.parent
+      JOIN moz_places h ON h.id = b.fk
+      WHERE h.url_hash = hash(:url) AND h.url = :url
+      AND _grandParentId <> :tagsFolderId
+      ORDER BY b.lastModified DESC
+      `, { url: info.url.href, tagsFolderId });
+
+    return rows.length ? rowsToItemsArray(rows) : null;
+  };
+
+  if (concurrent) {
+    let db = await PlacesUtils.promiseDBConnection();
+    return query(db);
+  }
   return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: fetchBookmarksByURL",
-    async function(db) {
-      let tagsFolderId = await promiseTagsFolderId();
-      let rows = await db.executeCached(
-        `/* do not warn (bug no): not worth to add an index */
-        SELECT b.guid, IFNULL(p.guid, "") AS parentGuid, b.position AS 'index',
-                b.dateAdded, b.lastModified, b.type, b.title, h.url AS url,
-                b.id AS _id, b.parent AS _parentId,
-                NULL AS _childCount, /* Unused for now */
-                p.parent AS _grandParentId, b.syncStatus AS _syncStatus
-        FROM moz_bookmarks b
-        JOIN moz_bookmarks p ON p.id = b.parent
-        JOIN moz_places h ON h.id = b.fk
-        WHERE h.url_hash = hash(:url) AND h.url = :url
-        AND _grandParentId <> :tagsFolderId
-        ORDER BY b.lastModified DESC
-        `, { url: info.url.href, tagsFolderId });
-
-      return rows.length ? rowsToItemsArray(rows) : null;
-    }
-  );
+                                           query);
 }
 
 function fetchRecentBookmarks(numberOfItems) {
   return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: fetchRecentBookmarks",
     async function(db) {
       let tagsFolderId = await promiseTagsFolderId();
       let rows = await db.executeCached(
         `SELECT b.guid, IFNULL(p.guid, "") AS parentGuid, b.position AS 'index',
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_fetch.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_fetch.js
@@ -302,11 +302,35 @@ add_task(async function fetch_byurl() {
   Assert.equal(gAccumulator.results.length, 2);
   gAccumulator.results.forEach(checkBookmarkObject);
   Assert.deepEqual(gAccumulator.results[0], bm5);
 
   // cleanup
   PlacesUtils.tagging.untagURI(uri(bm1.url.href), ["Test Tag"]);
 });
 
-function run_test() {
-  run_next_test();
-}
+add_task(async function fetch_concurrent() {
+  let bm1 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+                                                 url: "http://concurrent.url.com/" });
+  checkBookmarkObject(bm1);
+
+  let bm2 = await PlacesUtils.bookmarks.fetch({ url: bm1.url },
+                                              gAccumulator.callback,
+                                              { concurrent: true });
+  checkBookmarkObject(bm2);
+  Assert.equal(gAccumulator.results.length, 1);
+  Assert.deepEqual(gAccumulator.results[0], bm1);
+  Assert.deepEqual(bm1, bm2);
+  let bm3 = await PlacesUtils.bookmarks.fetch({ url: bm1.url },
+                                              gAccumulator.callback,
+                                              { concurrent: false });
+  checkBookmarkObject(bm3);
+  Assert.equal(gAccumulator.results.length, 1);
+  Assert.deepEqual(gAccumulator.results[0], bm1);
+  Assert.deepEqual(bm1, bm3);
+  let bm4 = await PlacesUtils.bookmarks.fetch({ url: bm1.url },
+                                              gAccumulator.callback,
+                                              {});
+  checkBookmarkObject(bm4);
+  Assert.equal(gAccumulator.results.length, 1);
+  Assert.deepEqual(gAccumulator.results[0], bm1);
+  Assert.deepEqual(bm1, bm4);
+});
--- a/toolkit/modules/Sqlite.jsm
+++ b/toolkit/modules/Sqlite.jsm
@@ -85,27 +85,26 @@ function logScriptError(message) {
   // flag can be used to suppress this for tests that explicitly
   // test auto closes.
   if (Debugging.failTestsOnAutoClose) {
     Promise.reject(new Error(message));
   }
 }
 
 /**
- * Gets connection identifier from its database file path.
+ * Gets connection identifier from its database file name.
  *
- * @param path
- *        A file string path pointing to a database file.
+ * @param fileName
+ *        A database file string name.
  * @return the connection identifier.
  */
-function getIdentifierByPath(path) {
-  let basename = OS.Path.basename(path);
-  let number = connectionCounters.get(basename) || 0;
-  connectionCounters.set(basename, number + 1);
-  return basename + "#" + number;
+function getIdentifierByFileName(fileName) {
+  let number = connectionCounters.get(fileName) || 0;
+  connectionCounters.set(fileName, number + 1);
+  return fileName + "#" + number;
 }
 
 /**
  * Barriers used to ensure that Sqlite.jsm is shutdown after all
  * its clients.
  */
 XPCOMUtils.defineLazyGetter(this, "Barriers", () => {
   let Barriers = {
@@ -210,17 +209,17 @@ XPCOMUtils.defineLazyGetter(this, "Barri
 function ConnectionData(connection, identifier, options = {}) {
   this._log = Log.repository.getLoggerWithMessagePrefix("Sqlite.Connection",
                                                         identifier + ": ");
   this._log.info("Opened");
 
   this._dbConn = connection;
 
   // This is a unique identifier for the connection, generated through
-  // getIdentifierByPath.  It may be used for logging or as a key in Maps.
+  // getIdentifierByFileName.  It may be used for logging or as a key in Maps.
   this._identifier = identifier;
 
   this._open = true;
 
   this._cachedStatements = new Map();
   this._anonymousStatements = new Map();
   this._anonymousCounter = 0;
 
@@ -927,17 +926,17 @@ function openConnection(options) {
                       "Got: " + options.shrinkMemoryOnConnectionIdleMS);
     }
 
     openedOptions.shrinkMemoryOnConnectionIdleMS =
       options.shrinkMemoryOnConnectionIdleMS;
   }
 
   let file = FileUtils.File(path);
-  let identifier = getIdentifierByPath(path);
+  let identifier = getIdentifierByFileName(OS.Path.basename(path));
 
   log.info("Opening database: " + path + " (" + identifier + ")");
 
   return new Promise((resolve, reject) => {
     let dbOptions = Cc["@mozilla.org/hash-property-bag;1"].
                     createInstance(Ci.nsIWritablePropertyBag);
     if (!sharedMemoryCache) {
       dbOptions.setProperty("shared", false);
@@ -1023,17 +1022,17 @@ function cloneStorageConnection(options)
       throw new TypeError("shrinkMemoryOnConnectionIdleMS must be an integer. " +
                           "Got: " + options.shrinkMemoryOnConnectionIdleMS);
     }
     openedOptions.shrinkMemoryOnConnectionIdleMS =
       options.shrinkMemoryOnConnectionIdleMS;
   }
 
   let path = source.databaseFile.path;
-  let identifier = getIdentifierByPath(path);
+  let identifier = getIdentifierByFileName(OS.Path.basename(path));
 
   log.info("Cloning database: " + path + " (" + identifier + ")");
 
   return new Promise((resolve, reject) => {
     source.asyncClone(!!options.readOnly, (status, connection) => {
       if (!connection) {
         log.warn("Could not clone connection: " + status);
         reject(new Error("Could not clone connection: " + status));
@@ -1076,20 +1075,19 @@ function wrapStorageConnection(options) 
   if (!connection || !(connection instanceof Ci.mozIStorageAsyncConnection)) {
     throw new TypeError("connection not specified or invalid.");
   }
 
   if (isClosed) {
     throw new Error("Sqlite.jsm has been shutdown. Cannot wrap connection to: " + connection.database.path);
   }
 
-  let path = connection.databaseFile.path;
-  let identifier = getIdentifierByPath(path);
+  let identifier = getIdentifierByFileName(connection.databaseFile.leafName);
 
-  log.info("Wrapping database: " + path + " (" + identifier + ")");
+  log.info("Wrapping database: " + identifier);
   return new Promise(resolve => {
     try {
       let conn = connection.QueryInterface(Ci.mozIStorageAsyncConnection);
       let wrapper = new OpenedConnection(conn, identifier);
       // We must not handle shutdown of a wrapped connection, since that is
       // already handled by the opener.
       wrappedConnections.add(identifier);
       resolve(wrapper);