Bug 834449 - Part 3: simplify cleanup of cached statements. r=gps
authorRichard Newman <rnewman@mozilla.com>
Fri, 25 Jan 2013 15:05:15 -0800
changeset 129852 2e916d9dae06eeeee1a4260a13ebece3def3249b
parent 129851 8efd1355f4acd9c646e3b7d853dd90e7fd8cef85
child 129853 aafdf3566582e9bf2635c03cf40f6173e7a79817
push id2323
push userbbajaj@mozilla.com
push dateMon, 01 Apr 2013 19:47:02 +0000
treeherdermozilla-beta@7712be144d91 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgps
bugs834449
milestone21.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 834449 - Part 3: simplify cleanup of cached statements. r=gps
toolkit/modules/Sqlite.jsm
toolkit/modules/tests/xpcshell/test_sqlite.js
--- a/toolkit/modules/Sqlite.jsm
+++ b/toolkit/modules/Sqlite.jsm
@@ -199,20 +199,16 @@ function OpenedConnection(connection, ba
   this._cachedStatements = new Map();
   this._anonymousStatements = new Map();
   this._anonymousCounter = 0;
 
   // A map from statement index to mozIStoragePendingStatement, to allow for
   // canceling prior to finalizing the mozIStorageStatements.
   this._pendingStatements = new Map();
 
-  // A map from statement index to mozIStorageStatement, for all outstanding
-  // query executions.
-  this._inProgressStatements = new Map();
-
   // Increments for each executed statement for the life of the connection.
   this._statementCounter = 0;
 
   this._inProgressTransaction = null;
 
   this._idleShrinkMS = options.shrinkMemoryOnConnectionIdleMS;
   if (this._idleShrinkMS) {
     this._idleShrinkTimer = Cc["@mozilla.org/timer;1"]
@@ -320,84 +316,25 @@ OpenedConnection.prototype = Object.free
 
     this.execute("ROLLBACK TRANSACTION").then(onRollback, onRollback);
     this._inProgressTransaction.reject(new Error("Connection being closed."));
     this._inProgressTransaction = null;
 
     return deferred.promise;
   },
 
-  /**
-   * Record that an executing statement has completed by removing it from the
-   * pending statements map and decrementing the corresponding in-progress
-   * counter.
-   */
-  _recordStatementCompleted: function (statement, index) {
-    this._log.debug("Stmt #" + index + " finished.");
-    this._pendingStatements.delete(index);
-
-    let now = this._inProgressStatements.get(statement) - 1;
-    if (now == 0) {
-      this._inProgressStatements.delete(statement);
-    } else {
-      this._inProgressStatements.set(statement, now);
-    }
-  },
-
-  /**
-   * Record that an executing statement is beginning by adding it to the
-   * pending statements map and incrementing the corresponding in-progress
-   * counter.
-   */
-  _recordStatementBeginning: function (statement, pending, index) {
-    this._log.info("Recording statement beginning: " + statement);
-    this._pendingStatements.set(index, pending);
-    if (!this._inProgressStatements.has(statement)) {
-      this._inProgressStatements.set(statement, 1);
-      this._log.info("Only one: " + statement);
-      return;
-    }
-    let now = this._inProgressStatements.get(statement) + 1;
-    this._inProgressStatements.set(statement, now);
-    this._log.info("More: " + statement + ", " + now);
-  },
-
-  /**
-   * Get a count of in-progress statements. This is useful for debugging and
-   * testing, and also for discarding cached statements.
-   *
-   * @param statement (optional) the statement after which to inquire.
-   * @return (integer) a count of executing queries, whether for `statement` or
-   *                   for the connection as a whole.
-   */
-  inProgress: function (statement) {
-    if (statement) {
-      if (this._inProgressStatements.has(statement)) {
-        return this._inProgressStatements.get(statement);
-      }
-      return 0;
-    }
-
-    let out = 0;
-    for (let v of this._inProgressStatements.values()) {
-      out += v;
-    }
-    return out;
-  },
-
   _finalize: function (deferred) {
     this._log.debug("Finalizing connection.");
     // Cancel any pending statements.
     for (let [k, statement] of this._pendingStatements) {
       statement.cancel();
     }
     this._pendingStatements.clear();
 
     // We no longer need to track these.
-    this._inProgressStatements.clear();
     this._statementCounter = 0;
 
     // Next we finalize all active statements.
     for (let [k, statement] of this._anonymousStatements) {
       statement.finalize();
     }
     this._anonymousStatements.clear();
 
@@ -727,31 +664,32 @@ OpenedConnection.prototype = Object.free
     this._log.info("Shrinking memory usage.");
 
     let onShrunk = this._clearIdleShrinkTimer.bind(this);
 
     return this.execute("PRAGMA shrink_memory").then(onShrunk, onShrunk);
   },
 
   /**
-   * Discard all inactive cached statements.
+   * Discard all cached statements.
+   *
+   * Note that this relies on us being non-interruptible between
+   * the insertion or retrieval of a statement in the cache and its
+   * execution: we finalize all statements, which is only safe if
+   * they will not be executed again.
    *
    * @return (integer) the number of statements discarded.
    */
   discardCachedStatements: function () {
     let count = 0;
     for (let [k, statement] of this._cachedStatements) {
-      if (this.inProgress(statement)) {
-        continue;
-      }
-
       ++count;
-      this._cachedStatements.delete(k);
       statement.finalize();
     }
+    this._cachedStatements.clear();
     this._log.debug("Discarded " + count + " cached statements.");
     return count;
   },
 
   _executeStatement: function (sql, statement, params, onRow) {
     if (statement.state != statement.MOZ_STORAGE_STATEMENT_READY) {
       throw new Error("Statement is not ready for execution.");
     }
@@ -819,17 +757,18 @@ OpenedConnection.prototype = Object.free
 
       handleError: function (error) {
         self._log.info("Error when executing SQL (" + error.result + "): " +
                        error.message);
         errors.push(error);
       },
 
       handleCompletion: function (reason) {
-        self._recordStatementCompleted(statement, index);
+        self._log.debug("Stmt #" + index + " finished.");
+        self._pendingStatements.delete(index);
 
         switch (reason) {
           case Ci.mozIStorageStatementCallback.REASON_FINISHED:
             // If there is an onRow handler, we always resolve to null.
             let result = onRow ? null : rows;
             deferred.resolve(result);
             break;
 
@@ -854,17 +793,17 @@ OpenedConnection.prototype = Object.free
           default:
             deferred.reject(new Error("Unknown completion reason code: " +
                                       reason));
             break;
         }
       },
     });
 
-    this._recordStatementBeginning(statement, pending, index);
+    this._pendingStatements.set(index, pending);
     return deferred.promise;
   },
 
   _ensureOpen: function () {
     if (!this._open) {
       throw new Error("Connection is not open.");
     }
   },
--- a/toolkit/modules/tests/xpcshell/test_sqlite.js
+++ b/toolkit/modules/tests/xpcshell/test_sqlite.js
@@ -473,43 +473,43 @@ add_task(function test_idle_shrink_reset
   yield shrinkPromises[0];
 
   yield c.close();
 });
 
 add_task(function test_in_progress_counts() {
   let c = yield getDummyDatabase("in_progress_counts");
   do_check_eq(c._statementCounter, c._initialStatementCount);
-  do_check_eq(c.inProgress(), 0);
+  do_check_eq(c._pendingStatements.size, 0);
   yield c.executeCached("INSERT INTO dirs (path) VALUES ('foo')");
   do_check_eq(c._statementCounter, c._initialStatementCount + 1);
-  do_check_eq(c.inProgress(), 0);
+  do_check_eq(c._pendingStatements.size, 0);
 
   let expectOne;
   let expectTwo;
 
   // Please forgive me.
   let inner = Async.makeSpinningCallback();
   let outer = Async.makeSpinningCallback();
 
   // We want to make sure that two queries executing simultaneously
-  // result in `inProgress()` reaching 2, then dropping back to 0.
+  // result in `_pendingStatements.size` reaching 2, then dropping back to 0.
   //
   // To do so, we kick off a second statement within the row handler
   // of the first, then wait for both to finish.
 
   yield c.executeCached("SELECT * from dirs", null, function onRow() {
     // In the onRow handler, we're still an outstanding query.
     // Expect a single in-progress entry.
-    expectOne = c.inProgress();
+    expectOne = c._pendingStatements.size;
 
     // Start another query, checking that after its statement has been created
     // there are two statements in progress.
     let p = c.executeCached("SELECT 10, path from dirs");
-    expectTwo = c.inProgress();
+    expectTwo = c._pendingStatements.size;
 
     // Now wait for it to be done before we return from the row handler …
     p.then(function onInner() {
       inner();
     });
   }).then(function onOuter() {
     // … and wait for the inner to be done before we finish …
     inner.wait();
@@ -518,17 +518,17 @@ add_task(function test_in_progress_count
 
   // … and wait for both queries to have finished before we go on and 
   // test postconditions.
   outer.wait();
 
   do_check_eq(expectOne, 1);
   do_check_eq(expectTwo, 2);
   do_check_eq(c._statementCounter, c._initialStatementCount + 3);
-  do_check_eq(c.inProgress(), 0);
+  do_check_eq(c._pendingStatements.size, 0);
 
   yield c.close();
 });
 
 add_task(function test_discard_while_active() {
   let c = yield getDummyDatabase("discard_while_active");
 
   yield c.executeCached("INSERT INTO dirs (path) VALUES ('foo')");
@@ -540,21 +540,18 @@ add_task(function test_discard_while_act
   yield c.executeCached(sql, null, function onRow(row) {
     if (!first) {
       return;
     }
     first = false;
     discarded = c.discardCachedStatements();
   });
 
-  // We didn't discard the SELECT, only the two INSERTs.
-  do_check_eq(2, discarded);
-
-  // But we got the remainder when it became inactive.
-  do_check_eq(1, c.discardCachedStatements());
+  // We discarded everything, because the SELECT had already started to run.
+  do_check_eq(3, discarded);
 
   // And again is safe.
   do_check_eq(0, c.discardCachedStatements());
 
   yield c.close();
 });
 
 add_task(function test_discard_cached() {