From 6c1628be1cc4839170b3e11e6a8520c80c18623e Mon Sep 17 00:00:00 2001 From: Richard Newman Date: Thu, 24 Jan 2013 11:10:18 -0800 Subject: [PATCH 1/8] Bug 834159 - Minor doc bugs in FHR. r=gps --- services/datareporting/DataReportingService.js | 2 +- services/healthreport/healthreporter.jsm | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/services/datareporting/DataReportingService.js b/services/datareporting/DataReportingService.js index 0bf81b05fad..0bca5e00e20 100644 --- a/services/datareporting/DataReportingService.js +++ b/services/datareporting/DataReportingService.js @@ -32,7 +32,7 @@ const DEFAULT_LOAD_DELAY_MSEC = 10 * 1000; * EXAMPLE USAGE * ============= * - * let reporter = Cc["@mozilla.org/healthreport/service;1"] + * let reporter = Cc["@mozilla.org/datareporting/service;1"] * .getService(Ci.nsISupports) * .wrappedJSObject * .healthReporter; diff --git a/services/healthreport/healthreporter.jsm b/services/healthreport/healthreporter.jsm index ebb61280920..37dc9a01d55 100644 --- a/services/healthreport/healthreporter.jsm +++ b/services/healthreport/healthreporter.jsm @@ -38,7 +38,8 @@ const DEFAULT_DATABASE_NAME = "healthreport.sqlite"; * lower-level components (such as collection and submission) together. * * An instance of this type is created as an XPCOM service. See - * HealthReportService.js and HealthReportComponents.manifest. + * DataReportingService.js and + * DataReporting.manifest/HealthReportComponents.manifest. * * It is theoretically possible to have multiple instances of this running * in the application. For example, this type may one day handle submission @@ -483,7 +484,7 @@ HealthReporter.prototype = Object.freeze({ * Register a `Metrics.Provider` with this instance. * * This needs to be called or no data will be collected. See also - * registerProvidersFromCategoryManager`. + * `registerProvidersFromCategoryManager`. * * @param provider * (Metrics.Provider) The provider to register for collection. From 640049a53fe04ca398e9184ba8c1ecacf4f84abc Mon Sep 17 00:00:00 2001 From: Gregory Szorc Date: Thu, 24 Jan 2013 11:10:19 -0800 Subject: [PATCH 2/8] Bug 833609 - Part 1: add a manual call to shrink memory usage to Sqlite.jsm. r=mak --- toolkit/modules/Sqlite.jsm | 9 +++++++++ toolkit/modules/tests/xpcshell/test_sqlite.js | 9 +++++++++ 2 files changed, 18 insertions(+) diff --git a/toolkit/modules/Sqlite.jsm b/toolkit/modules/Sqlite.jsm index ead715b1a1b..ca9aa6c9a3c 100644 --- a/toolkit/modules/Sqlite.jsm +++ b/toolkit/modules/Sqlite.jsm @@ -586,6 +586,15 @@ OpenedConnection.prototype = Object.freeze({ ); }, + /** + * Free up as much memory from the underlying database connection as possible. + * + * @return Promise<> + */ + shrinkMemory: function () { + return this.execute("PRAGMA shrink_memory"); + }, + _executeStatement: function (sql, statement, params, onRow) { if (statement.state != statement.MOZ_STORAGE_STATEMENT_READY) { throw new Error("Statement is not ready for execution."); diff --git a/toolkit/modules/tests/xpcshell/test_sqlite.js b/toolkit/modules/tests/xpcshell/test_sqlite.js index 1f4455332d3..3dc4a8cde1c 100644 --- a/toolkit/modules/tests/xpcshell/test_sqlite.js +++ b/toolkit/modules/tests/xpcshell/test_sqlite.js @@ -323,3 +323,12 @@ add_task(function test_detect_multiple_transactions() { yield c.close(); }); +add_task(function test_shrink_memory() { + let c = yield getDummyDatabase("shrink_memory"); + + // It's just a simple sanity test. We have no way of measuring whether this + // actually does anything. + + yield c.shrinkMemory(); + yield c.close(); +}); From 5e9f7129d10f62f349dc08379c0bd14ee81a4a52 Mon Sep 17 00:00:00 2001 From: Gregory Szorc Date: Thu, 24 Jan 2013 13:30:20 -0800 Subject: [PATCH 3/8] Bug 833609 - Part 2: Add timer to shrink memory after idle; r=mak --- toolkit/modules/Sqlite.jsm | 119 +++++++++++--- toolkit/modules/tests/xpcshell/test_sqlite.js | 151 +++++++++++++++++- 2 files changed, 246 insertions(+), 24 deletions(-) diff --git a/toolkit/modules/Sqlite.jsm b/toolkit/modules/Sqlite.jsm index ca9aa6c9a3c..279f75aab59 100644 --- a/toolkit/modules/Sqlite.jsm +++ b/toolkit/modules/Sqlite.jsm @@ -8,7 +8,7 @@ this.EXPORTED_SYMBOLS = [ "Sqlite", ]; -const {interfaces: Ci, utils: Cu} = Components; +const {classes: Cc, interfaces: Ci, utils: Cu} = Components; Cu.import("resource://gre/modules/commonjs/promise/core.js"); Cu.import("resource://gre/modules/osfile.jsm"); @@ -43,6 +43,13 @@ let connectionCounters = {}; * to obtain a lock, possibly making database access slower. Defaults to * true. * + * shrinkMemoryOnConnectionIdleMS -- (integer) If defined, the connection + * will attempt to minimize its memory usage after this many + * milliseconds of connection idle. The connection is idle when no + * statements are executing. There is no default value which means no + * automatic memory minimization will occur. Please note that this is + * *not* a timer on the idle service and this could fire while the + * application is active. * * FUTURE options to control: * @@ -69,6 +76,18 @@ function openConnection(options) { let sharedMemoryCache = "sharedMemoryCache" in options ? options.sharedMemoryCache : true; + let openedOptions = {}; + + if ("shrinkMemoryOnConnectionIdleMS" in options) { + if (!Number.isInteger(options.shrinkMemoryOnConnectionIdleMS)) { + throw new Error("shrinkMemoryOnConnectionIdleMS must be an integer. " + + "Got: " + options.shrinkMemoryOnConnectionIdleMS); + } + + openedOptions.shrinkMemoryOnConnectionIdleMS = + options.shrinkMemoryOnConnectionIdleMS; + } + let file = FileUtils.File(path); let openDatabaseFn = sharedMemoryCache ? Services.storage.openDatabase : @@ -92,7 +111,8 @@ function openConnection(options) { return Promise.reject(new Error("Connection is not ready.")); } - return Promise.resolve(new OpenedConnection(connection, basename, number)); + return Promise.resolve(new OpenedConnection(connection, basename, number, + openedOptions)); } catch (ex) { log.warn("Could not open database: " + CommonUtils.exceptionStr(ex)); return Promise.reject(ex); @@ -143,8 +163,11 @@ function openConnection(options) { * (string) The basename of this database name. Used for logging. * @param number * (Number) The connection number to this database. + * @param options + * (object) Options to control behavior of connection. See + * `openConnection`. */ -function OpenedConnection(connection, basename, number) { +function OpenedConnection(connection, basename, number, options) { let log = Log4Moz.repository.getLogger("Sqlite.Connection." + basename); // getLogger() returns a shared object. We can't modify the functions on this @@ -180,6 +203,14 @@ function OpenedConnection(connection, basename, number) { this._inProgressCounter = 0; this._inProgressTransaction = null; + + this._idleShrinkMS = options.shrinkMemoryOnConnectionIdleMS; + if (this._idleShrinkMS) { + this._idleShrinkTimer = Cc["@mozilla.org/timer;1"] + .createInstance(Ci.nsITimer); + // We wait for the first statement execute to start the timer because + // shrinking now would not do anything. + } } OpenedConnection.prototype = Object.freeze({ @@ -259,7 +290,7 @@ OpenedConnection.prototype = Object.freeze({ } this._log.debug("Request to close connection."); - + this._clearIdleShrinkTimer(); let deferred = Promise.defer(); // We need to take extra care with transactions during shutdown. @@ -389,7 +420,27 @@ OpenedConnection.prototype = Object.freeze({ this._cachedStatements.set(sql, statement); } - return this._executeStatement(sql, statement, params, onRow); + this._clearIdleShrinkTimer(); + + let deferred = Promise.defer(); + + try { + this._executeStatement(sql, statement, params, onRow).then( + function onResult(result) { + this._startIdleShrinkTimer(); + deferred.resolve(result); + }.bind(this), + function onError(error) { + this._startIdleShrinkTimer(); + deferred.reject(error); + }.bind(this) + ); + } catch (ex) { + this._startIdleShrinkTimer(); + throw ex; + } + + return deferred.promise; }, /** @@ -418,22 +469,32 @@ OpenedConnection.prototype = Object.freeze({ let index = this._anonymousCounter++; this._anonymousStatements.set(index, statement); + this._clearIdleShrinkTimer(); + + let onFinished = function () { + this._anonymousStatements.delete(index); + statement.finalize(); + this._startIdleShrinkTimer(); + }.bind(this); let deferred = Promise.defer(); - this._executeStatement(sql, statement, params, onRow).then( - function onResult(rows) { - this._anonymousStatements.delete(index); - statement.finalize(); - deferred.resolve(rows); - }.bind(this), + try { + this._executeStatement(sql, statement, params, onRow).then( + function onResult(rows) { + onFinished(); + deferred.resolve(rows); + }.bind(this), - function onError(error) { - this._anonymousStatements.delete(index); - statement.finalize(); - deferred.reject(error); - }.bind(this) - ); + function onError(error) { + onFinished(); + deferred.reject(error); + }.bind(this) + ); + } catch (ex) { + onFinished(); + throw ex; + } return deferred.promise; }, @@ -592,7 +653,11 @@ OpenedConnection.prototype = Object.freeze({ * @return Promise<> */ shrinkMemory: function () { - return this.execute("PRAGMA shrink_memory"); + this._log.info("Shrinking memory usage."); + + let onShrunk = this._clearIdleShrinkTimer.bind(this); + + return this.execute("PRAGMA shrink_memory").then(onShrunk, onShrunk); }, _executeStatement: function (sql, statement, params, onRow) { @@ -714,6 +779,24 @@ OpenedConnection.prototype = Object.freeze({ throw new Error("Connection is not open."); } }, + + _clearIdleShrinkTimer: function () { + if (!this._idleShrinkTimer) { + return; + } + + this._idleShrinkTimer.cancel(); + }, + + _startIdleShrinkTimer: function () { + if (!this._idleShrinkTimer) { + return; + } + + this._idleShrinkTimer.initWithCallback(this.shrinkMemory.bind(this), + this._idleShrinkMS, + this._idleShrinkTimer.TYPE_ONE_SHOT); + }, }); this.Sqlite = { diff --git a/toolkit/modules/tests/xpcshell/test_sqlite.js b/toolkit/modules/tests/xpcshell/test_sqlite.js index 3dc4a8cde1c..05b9172cd2a 100644 --- a/toolkit/modules/tests/xpcshell/test_sqlite.js +++ b/toolkit/modules/tests/xpcshell/test_sqlite.js @@ -3,7 +3,7 @@ "use strict"; -const {utils: Cu} = Components; +const {classes: Cc, interfaces: Ci, utils: Cu} = Components; do_get_profile(); @@ -13,19 +13,38 @@ Cu.import("resource://gre/modules/Sqlite.jsm"); Cu.import("resource://gre/modules/Task.jsm"); -function getConnection(dbName) { - let path = dbName + ".sqlite"; +function sleep(ms) { + let deferred = Promise.defer(); - return Sqlite.openConnection({path: path}); + let timer = Cc["@mozilla.org/timer;1"] + .createInstance(Ci.nsITimer); + + timer.initWithCallback({ + notify: function () { + deferred.resolve(); + }, + }, ms, timer.TYPE_ONE_SHOT); + + return deferred.promise; } -function getDummyDatabase(name) { +function getConnection(dbName, extraOptions={}) { + let path = dbName + ".sqlite"; + let options = {path: path}; + for (let [k, v] in Iterator(extraOptions)) { + options[k] = v; + } + + return Sqlite.openConnection(options); +} + +function getDummyDatabase(name, extraOptions={}) { const TABLES = { dirs: "id INTEGER PRIMARY KEY AUTOINCREMENT, path TEXT", files: "id INTEGER PRIMARY KEY AUTOINCREMENT, dir_id INTEGER, path TEXT", }; - let c = yield getConnection(name); + let c = yield getConnection(name, extraOptions); for (let [k, v] in Iterator(TABLES)) { yield c.execute("CREATE TABLE " + k + "(" + v + ")"); @@ -161,11 +180,17 @@ add_task(function test_execute_invalid_statement() { let deferred = Promise.defer(); + do_check_eq(c._anonymousStatements.size, 0); + c.execute("SELECT invalid FROM unknown").then(do_throw, function onError(error) { deferred.resolve(); }); yield deferred.promise; + + // Ensure we don't leak the statement instance. + do_check_eq(c._anonymousStatements.size, 0); + yield c.close(); }); @@ -332,3 +357,117 @@ add_task(function test_shrink_memory() { yield c.shrinkMemory(); yield c.close(); }); + +add_task(function test_no_shrink_on_init() { + let c = yield getConnection("no_shrink_on_init", + {shrinkMemoryOnConnectionIdleMS: 200}); + + let oldShrink = c.shrinkMemory; + let count = 0; + Object.defineProperty(c, "shrinkMemory", { + value: function () { + count++; + }, + }); + + // We should not shrink until a statement has been executed. + yield sleep(220); + do_check_eq(count, 0); + + yield c.execute("SELECT 1"); + yield sleep(220); + do_check_eq(count, 1); + + yield c.close(); +}); + +add_task(function test_idle_shrink_fires() { + let c = yield getDummyDatabase("idle_shrink_fires", + {shrinkMemoryOnConnectionIdleMS: 200}); + c._clearIdleShrinkTimer(); + + let oldShrink = c.shrinkMemory; + let shrinkPromises = []; + + let count = 0; + Object.defineProperty(c, "shrinkMemory", { + value: function () { + count++; + let promise = oldShrink.call(c); + shrinkPromises.push(promise); + return promise; + }, + }); + + // We reset the idle shrink timer after monkeypatching because otherwise the + // installed timer callback will reference the non-monkeypatched function. + c._startIdleShrinkTimer(); + + yield sleep(220); + do_check_eq(count, 1); + do_check_eq(shrinkPromises.length, 1); + yield shrinkPromises[0]; + shrinkPromises.shift(); + + // We shouldn't shrink again unless a statement was executed. + yield sleep(300); + do_check_eq(count, 1); + + yield c.execute("SELECT 1"); + yield sleep(300); + + do_check_eq(count, 2); + do_check_eq(shrinkPromises.length, 1); + yield shrinkPromises[0]; + + yield c.close(); +}); + +add_task(function test_idle_shrink_reset_on_operation() { + const INTERVAL = 500; + let c = yield getDummyDatabase("idle_shrink_reset_on_operation", + {shrinkMemoryOnConnectionIdleMS: INTERVAL}); + + c._clearIdleShrinkTimer(); + + let oldShrink = c.shrinkMemory; + let shrinkPromises = []; + let count = 0; + + Object.defineProperty(c, "shrinkMemory", { + value: function () { + count++; + let promise = oldShrink.call(c); + shrinkPromises.push(promise); + return promise; + }, + }); + + let now = new Date(); + c._startIdleShrinkTimer(); + + let initialIdle = new Date(now.getTime() + INTERVAL); + + // Perform database operations until initial scheduled time has been passed. + let i = 0; + while (new Date() < initialIdle) { + yield c.execute("INSERT INTO dirs (path) VALUES (?)", ["" + i]); + i++; + } + + do_check_true(i > 0); + + // We should not have performed an idle while doing operations. + do_check_eq(count, 0); + + // Wait for idle timer. + yield sleep(INTERVAL); + + // Ensure we fired. + do_check_eq(count, 1); + do_check_eq(shrinkPromises.length, 1); + yield shrinkPromises[0]; + + yield c.close(); +}); + From f6a6ee2d47778bc35c81b15465081333b7c98bff Mon Sep 17 00:00:00 2001 From: Richard Newman Date: Fri, 25 Jan 2013 00:32:33 -0800 Subject: [PATCH 4/8] Bug 830922 - Include version inside measurement payload. r=gps --- services/healthreport/healthreporter.jsm | 4 +++- services/healthreport/providers.jsm | 6 ++++-- services/healthreport/tests/xpcshell/test_healthreporter.js | 4 +++- .../healthreport/tests/xpcshell/test_provider_addons.js | 3 ++- .../healthreport/tests/xpcshell/test_provider_appinfo.js | 1 + .../healthreport/tests/xpcshell/test_provider_sessions.js | 1 + .../healthreport/tests/xpcshell/test_provider_sysinfo.js | 1 + services/metrics/dataprovider.jsm | 4 ++-- services/metrics/tests/xpcshell/test_metrics_provider.js | 6 ++++-- 9 files changed, 21 insertions(+), 9 deletions(-) diff --git a/services/healthreport/healthreporter.jsm b/services/healthreport/healthreporter.jsm index 37dc9a01d55..2104f2e0ad7 100644 --- a/services/healthreport/healthreporter.jsm +++ b/services/healthreport/healthreporter.jsm @@ -627,10 +627,12 @@ HealthReporter.prototype = Object.freeze({ }; for (let [measurementKey, measurement] of provider.measurements) { - let name = providerName + "." + measurement.name + "." + measurement.version; + let name = providerName + "." + measurement.name; let serializer; try { + // The measurement is responsible for returning a serializer which + // is aware of the measurement version. serializer = measurement.serializer(measurement.SERIALIZE_JSON); } catch (ex) { this._log.warn("Error obtaining serializer for measurement: " + name + diff --git a/services/healthreport/providers.jsm b/services/healthreport/providers.jsm index 10dfeb9207e..4242fc35469 100644 --- a/services/healthreport/providers.jsm +++ b/services/healthreport/providers.jsm @@ -394,7 +394,7 @@ CurrentSessionMeasurement.prototype = Object.freeze({ }, _serializeJSONSingular: function (data) { - let result = {}; + let result = {"_v": this.version}; for (let [field, value] of data) { result[field] = value[1]; @@ -535,7 +535,9 @@ ActiveAddonsMeasurement.prototype = Object.freeze({ } // Exceptions are caught in the caller. - return JSON.parse(data.get("addons")[1]); + let result = JSON.parse(data.get("addons")[1]); + result._v = this.version; + return result; }, }); diff --git a/services/healthreport/tests/xpcshell/test_healthreporter.js b/services/healthreport/tests/xpcshell/test_healthreporter.js index fdabf113243..76d0d113529 100644 --- a/services/healthreport/tests/xpcshell/test_healthreporter.js +++ b/services/healthreport/tests/xpcshell/test_healthreporter.js @@ -204,8 +204,10 @@ add_task(function test_json_payload_dummy_provider() { print(payload); let o = JSON.parse(payload); + let name = "DummyProvider.DummyMeasurement"; do_check_eq(Object.keys(o.data.last).length, 1); - do_check_true("DummyProvider.DummyMeasurement.1" in o.data.last); + do_check_true(name in o.data.last); + do_check_eq(o.data.last[name]._v, 1); reporter._shutdown(); }); diff --git a/services/healthreport/tests/xpcshell/test_provider_addons.js b/services/healthreport/tests/xpcshell/test_provider_addons.js index cee1fd2c91e..b2d8e482b98 100644 --- a/services/healthreport/tests/xpcshell/test_provider_addons.js +++ b/services/healthreport/tests/xpcshell/test_provider_addons.js @@ -111,9 +111,10 @@ add_task(function test_collect() { let serializer = active.serializer(active.SERIALIZE_JSON); let serialized = serializer.singular(data.singular); do_check_eq(typeof(serialized), "object"); - do_check_eq(Object.keys(serialized).length, 2); + do_check_eq(Object.keys(serialized).length, 3); // Our two keys, plus _v. do_check_true("addon0" in serialized); do_check_true("addon1" in serialized); + do_check_eq(serialized._v, 1); let counts = provider.getMeasurement("counts", 1); data = yield counts.getValues(); diff --git a/services/healthreport/tests/xpcshell/test_provider_appinfo.js b/services/healthreport/tests/xpcshell/test_provider_appinfo.js index a2804c07aad..06925c8534d 100644 --- a/services/healthreport/tests/xpcshell/test_provider_appinfo.js +++ b/services/healthreport/tests/xpcshell/test_provider_appinfo.js @@ -34,6 +34,7 @@ add_task(function test_collect_smoketest() { let serializer = m.serializer(m.SERIALIZE_JSON); let d = serializer.singular(data.singular); + do_check_eq(d._v, 1); do_check_eq(d.vendor, "Mozilla"); do_check_eq(d.name, "xpcshell"); do_check_eq(d.id, "xpcshell@tests.mozilla.org"); diff --git a/services/healthreport/tests/xpcshell/test_provider_sessions.js b/services/healthreport/tests/xpcshell/test_provider_sessions.js index 8c867115f3a..f3cc5ec1dd4 100644 --- a/services/healthreport/tests/xpcshell/test_provider_sessions.js +++ b/services/healthreport/tests/xpcshell/test_provider_sessions.js @@ -170,6 +170,7 @@ add_task(function test_serialization() { let serializer = current.serializer(current.SERIALIZE_JSON); let fields = serializer.singular(data.singular); + do_check_eq(fields._v, 2); do_check_eq(fields.activeTicks, 0); do_check_eq(fields.startDay, Metrics.dateToDays(recorder.startDate)); do_check_eq(fields.main, 500); diff --git a/services/healthreport/tests/xpcshell/test_provider_sysinfo.js b/services/healthreport/tests/xpcshell/test_provider_sysinfo.js index db01fc5d19b..537e5e65366 100644 --- a/services/healthreport/tests/xpcshell/test_provider_sysinfo.js +++ b/services/healthreport/tests/xpcshell/test_provider_sysinfo.js @@ -32,6 +32,7 @@ add_task(function test_collect_smoketest() { let serializer = m.serializer(m.SERIALIZE_JSON); let d = serializer.singular(data.singular); + do_check_eq(d._v, 1); do_check_true(d.cpuCount > 0); do_check_neq(d.name, null); diff --git a/services/metrics/dataprovider.jsm b/services/metrics/dataprovider.jsm index de190d071d8..ab485d814c0 100644 --- a/services/metrics/dataprovider.jsm +++ b/services/metrics/dataprovider.jsm @@ -246,7 +246,7 @@ Measurement.prototype = Object.freeze({ }, _serializeJSONSingular: function (data) { - let result = {}; + let result = {"_v": this.version}; for (let [field, data] of data) { // There could be legacy fields in storage we no longer care about. @@ -278,7 +278,7 @@ Measurement.prototype = Object.freeze({ }, _serializeJSONDay: function (data) { - let result = {}; + let result = {"_v": this.version}; for (let [field, data] of data) { if (!this._fieldsByName.has(field)) { diff --git a/services/metrics/tests/xpcshell/test_metrics_provider.js b/services/metrics/tests/xpcshell/test_metrics_provider.js index 5042a624471..15590d7504c 100644 --- a/services/metrics/tests/xpcshell/test_metrics_provider.js +++ b/services/metrics/tests/xpcshell/test_metrics_provider.js @@ -256,15 +256,17 @@ add_task(function test_serialize_json_default() { let serializer = m.serializer(m.SERIALIZE_JSON); let formatted = serializer.singular(data.singular); - do_check_eq(Object.keys(formatted).length, 2); + do_check_eq(Object.keys(formatted).length, 3); // Our keys + _v. do_check_true("last-numeric" in formatted); do_check_true("last-text" in formatted); do_check_eq(formatted["last-numeric"], 6); do_check_eq(formatted["last-text"], "hello"); + do_check_eq(formatted["_v"], 1); formatted = serializer.daily(data.days.getDay(now)); - do_check_eq(Object.keys(formatted).length, 5); + do_check_eq(Object.keys(formatted).length, 6); // Our keys + _v. do_check_eq(formatted["daily-counter"], 2); + do_check_eq(formatted["_v"], 1); do_check_true(Array.isArray(formatted["daily-discrete-numeric"])); do_check_eq(formatted["daily-discrete-numeric"].length, 2); From c80442d777d605b1be821993764235f1fc532062 Mon Sep 17 00:00:00 2001 From: Richard Newman Date: Fri, 25 Jan 2013 00:35:19 -0800 Subject: [PATCH 5/8] Bug 834449 - Part 1: rework tracking of pending statements. r=gps --- toolkit/modules/Sqlite.jsm | 85 +++++++++++++++++-- toolkit/modules/tests/xpcshell/test_sqlite.js | 57 +++++++++++++ 2 files changed, 134 insertions(+), 8 deletions(-) diff --git a/toolkit/modules/Sqlite.jsm b/toolkit/modules/Sqlite.jsm index 279f75aab59..d1909318300 100644 --- a/toolkit/modules/Sqlite.jsm +++ b/toolkit/modules/Sqlite.jsm @@ -199,8 +199,17 @@ function OpenedConnection(connection, basename, number, options) { 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(); - this._inProgressCounter = 0; + + // Increments for each executed statement for the life of the connection. + this._statementCounter = 0; this._inProgressTransaction = null; @@ -316,13 +325,75 @@ OpenedConnection.prototype = Object.freeze({ 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 in-progress statements. - for (let [k, statement] of this._inProgressStatements) { + // 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) { @@ -682,7 +753,7 @@ OpenedConnection.prototype = Object.freeze({ "object. Got: " + params); } - let index = this._inProgressCounter++; + let index = this._statementCounter++; let deferred = Promise.defer(); let userCancelled = false; @@ -733,8 +804,7 @@ OpenedConnection.prototype = Object.freeze({ }, handleCompletion: function (reason) { - self._log.debug("Stmt #" + index + " finished"); - self._inProgressStatements.delete(index); + self._recordStatementCompleted(statement, index); switch (reason) { case Ci.mozIStorageStatementCallback.REASON_FINISHED: @@ -769,8 +839,7 @@ OpenedConnection.prototype = Object.freeze({ }, }); - this._inProgressStatements.set(index, pending); - + this._recordStatementBeginning(statement, pending, index); return deferred.promise; }, diff --git a/toolkit/modules/tests/xpcshell/test_sqlite.js b/toolkit/modules/tests/xpcshell/test_sqlite.js index 05b9172cd2a..dc88b0dbe33 100644 --- a/toolkit/modules/tests/xpcshell/test_sqlite.js +++ b/toolkit/modules/tests/xpcshell/test_sqlite.js @@ -12,6 +12,8 @@ Cu.import("resource://gre/modules/osfile.jsm"); Cu.import("resource://gre/modules/Sqlite.jsm"); Cu.import("resource://gre/modules/Task.jsm"); +// To spin the event loop in test. +Cu.import("resource://services-common/async.js"); function sleep(ms) { let deferred = Promise.defer(); @@ -45,9 +47,11 @@ function getDummyDatabase(name, extraOptions={}) { }; let c = yield getConnection(name, extraOptions); + c._initialStatementCount = 0; for (let [k, v] in Iterator(TABLES)) { yield c.execute("CREATE TABLE " + k + "(" + v + ")"); + c._initialStatementCount++; } throw new Task.Result(c); @@ -471,3 +475,56 @@ add_task(function test_idle_shrink_reset_on_operation() { 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); + yield c.executeCached("INSERT INTO dirs (path) VALUES ('foo')"); + do_check_eq(c._statementCounter, c._initialStatementCount + 1); + do_check_eq(c.inProgress(), 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. + // + // 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(); + + // 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(); + + // 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(); + outer(); + }); + + // … 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); + + yield c.close(); +}); + From d27027b2fdfcbe8a32a55a2782f6cdf2e495286a Mon Sep 17 00:00:00 2001 From: Richard Newman Date: Fri, 25 Jan 2013 00:38:59 -0800 Subject: [PATCH 6/8] Bug 834449 - Part 2: clean up cached statements in Sqlite.jsm. r=gps --- toolkit/modules/Sqlite.jsm | 20 ++++++++ toolkit/modules/tests/xpcshell/test_sqlite.js | 47 +++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/toolkit/modules/Sqlite.jsm b/toolkit/modules/Sqlite.jsm index d1909318300..452a4e11f78 100644 --- a/toolkit/modules/Sqlite.jsm +++ b/toolkit/modules/Sqlite.jsm @@ -731,6 +731,26 @@ OpenedConnection.prototype = Object.freeze({ return this.execute("PRAGMA shrink_memory").then(onShrunk, onShrunk); }, + /** + * Discard all inactive cached statements. + * + * @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._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."); diff --git a/toolkit/modules/tests/xpcshell/test_sqlite.js b/toolkit/modules/tests/xpcshell/test_sqlite.js index dc88b0dbe33..c2d2b176ad5 100644 --- a/toolkit/modules/tests/xpcshell/test_sqlite.js +++ b/toolkit/modules/tests/xpcshell/test_sqlite.js @@ -528,3 +528,50 @@ add_task(function test_in_progress_counts() { 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')"); + yield c.executeCached("INSERT INTO dirs (path) VALUES ('bar')"); + + let discarded = -1; + let first = true; + let sql = "SELECT * FROM dirs"; + 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()); + + // And again is safe. + do_check_eq(0, c.discardCachedStatements()); + + yield c.close(); +}); + +add_task(function test_discard_cached() { + let c = yield getDummyDatabase("discard_cached"); + + yield c.executeCached("SELECT * from dirs"); + do_check_eq(1, c._cachedStatements.size); + + yield c.executeCached("SELECT * from files"); + do_check_eq(2, c._cachedStatements.size); + + yield c.executeCached("SELECT * from dirs"); + do_check_eq(2, c._cachedStatements.size); + + c.discardCachedStatements(); + do_check_eq(0, c._cachedStatements.size); + + yield c.close(); +}); + From ae72dcf6074e8983814fdf076679d3a01216b945 Mon Sep 17 00:00:00 2001 From: Richard Newman Date: Fri, 25 Jan 2013 00:39:01 -0800 Subject: [PATCH 7/8] Bug 832067 - Discard cached statements in healthreporter. r=gps --- services/healthreport/healthreporter.jsm | 4 ++++ services/metrics/storage.jsm | 15 +++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/services/healthreport/healthreporter.jsm b/services/healthreport/healthreporter.jsm index 2104f2e0ad7..482d08f4e48 100644 --- a/services/healthreport/healthreporter.jsm +++ b/services/healthreport/healthreporter.jsm @@ -307,6 +307,9 @@ HealthReporter.prototype = Object.freeze({ this._log.info("HealthReporter started."); this._initialized = true; Services.obs.addObserver(this, "idle-daily", false); + + // Clean up caches and reduce memory usage. + this._storage.compact(); this._initializedDeferred.resolve(this); }, @@ -695,6 +698,7 @@ HealthReporter.prototype = Object.freeze({ o.errors = errors; } + this._storage.compact(); throw new Task.Result(JSON.stringify(o)); }, diff --git a/services/metrics/storage.jsm b/services/metrics/storage.jsm index 1f590b22996..7dd432fa4a4 100644 --- a/services/metrics/storage.jsm +++ b/services/metrics/storage.jsm @@ -1204,6 +1204,21 @@ MetricsStorageSqliteBackend.prototype = Object.freeze({ }); }, + /** + * Reduce memory usage as much as possible. + * + * This returns a promise that will be resolved on completion. + * + * @return Promise<> + */ + compact: function () { + let self = this; + return this.enqueueOperation(function doCompact() { + self._connection.discardCachedStatements(); + return self._connection.shrinkMemory(); + }); + }, + /** * Ensure a field ID matches a specified type. * From e98491e7fe6ea5f3034d7dde5304b848ad8ac6ce Mon Sep 17 00:00:00 2001 From: Richard Newman Date: Fri, 25 Jan 2013 15:05:15 -0800 Subject: [PATCH 8/8] Bug 834449 - Part 3: simplify cleanup of cached statements. r=gps --- toolkit/modules/Sqlite.jsm | 81 +++---------------- toolkit/modules/tests/xpcshell/test_sqlite.js | 19 ++--- 2 files changed, 18 insertions(+), 82 deletions(-) diff --git a/toolkit/modules/Sqlite.jsm b/toolkit/modules/Sqlite.jsm index 452a4e11f78..a487c3e39d9 100644 --- a/toolkit/modules/Sqlite.jsm +++ b/toolkit/modules/Sqlite.jsm @@ -204,10 +204,6 @@ function OpenedConnection(connection, basename, number, options) { // 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; @@ -325,64 +321,6 @@ OpenedConnection.prototype = Object.freeze({ 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. @@ -392,7 +330,6 @@ OpenedConnection.prototype = Object.freeze({ this._pendingStatements.clear(); // We no longer need to track these. - this._inProgressStatements.clear(); this._statementCounter = 0; // Next we finalize all active statements. @@ -732,21 +669,22 @@ OpenedConnection.prototype = Object.freeze({ }, /** - * 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; }, @@ -824,7 +762,8 @@ OpenedConnection.prototype = Object.freeze({ }, handleCompletion: function (reason) { - self._recordStatementCompleted(statement, index); + self._log.debug("Stmt #" + index + " finished."); + self._pendingStatements.delete(index); switch (reason) { case Ci.mozIStorageStatementCallback.REASON_FINISHED: @@ -859,7 +798,7 @@ OpenedConnection.prototype = Object.freeze({ }, }); - this._recordStatementBeginning(statement, pending, index); + this._pendingStatements.set(index, pending); return deferred.promise; }, diff --git a/toolkit/modules/tests/xpcshell/test_sqlite.js b/toolkit/modules/tests/xpcshell/test_sqlite.js index c2d2b176ad5..aed7e966475 100644 --- a/toolkit/modules/tests/xpcshell/test_sqlite.js +++ b/toolkit/modules/tests/xpcshell/test_sqlite.js @@ -478,10 +478,10 @@ add_task(function test_idle_shrink_reset_on_operation() { 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; @@ -491,7 +491,7 @@ add_task(function test_in_progress_counts() { 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. @@ -499,12 +499,12 @@ add_task(function test_in_progress_counts() { 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() { @@ -523,7 +523,7 @@ add_task(function test_in_progress_counts() { 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(); }); @@ -545,11 +545,8 @@ add_task(function test_discard_while_active() { 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());