From ea427892068078779c9c2fc499458b1745c3e512 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Wed, 21 Apr 2021 17:33:24 -0400 Subject: [PATCH] Avoid memory leaks from caching unique DB statements Many DB statements that were being cached included embedded ids or temp table names, so they would permanently use memory and never get reused. During large imports, this could result in hundreds of megabytes of extra memory usage. This commit marks many more statements as `noCache`, adds `noCache` support for column and value queries, adds a log line at DB shutdown in source builds with the number of cached statements, and adds Zotero.DB.getCachedStatements() for auditing of cached statements. Addresses #1531 --- .../content/zotero/xpcom/collectionTreeRow.js | 2 +- .../zotero/xpcom/collectionTreeView.js | 4 +- .../content/zotero/xpcom/data/dataObjects.js | 3 +- chrome/content/zotero/xpcom/data/items.js | 27 +++++++------- chrome/content/zotero/xpcom/data/search.js | 26 ++++++++----- chrome/content/zotero/xpcom/data/tags.js | 21 ++++++++--- chrome/content/zotero/xpcom/db.js | 37 ++++++++++++++++++- 7 files changed, 86 insertions(+), 34 deletions(-) diff --git a/chrome/content/zotero/xpcom/collectionTreeRow.js b/chrome/content/zotero/xpcom/collectionTreeRow.js index 57f282dfd8..9b5bf2c30f 100644 --- a/chrome/content/zotero/xpcom/collectionTreeRow.js +++ b/chrome/content/zotero/xpcom/collectionTreeRow.js @@ -327,7 +327,7 @@ Zotero.CollectionTreeRow.prototype.getSearchObject = Zotero.Promise.coroutine(fu } // Called by ItemTreeView::unregister() this.onUnload = async function () { - await Zotero.DB.queryAsync(`DROP TABLE IF EXISTS ${tmpTable}`); + await Zotero.DB.queryAsync(`DROP TABLE IF EXISTS ${tmpTable}`, false, { noCache: true }); }; } else { diff --git a/chrome/content/zotero/xpcom/collectionTreeView.js b/chrome/content/zotero/xpcom/collectionTreeView.js index 5f41236157..e786224137 100644 --- a/chrome/content/zotero/xpcom/collectionTreeView.js +++ b/chrome/content/zotero/xpcom/collectionTreeView.js @@ -2474,7 +2474,9 @@ Zotero.CollectionTreeCache = { if (this.lastTempTable) { let tableName = this.lastTempTable; let id = Zotero.DB.addCallback('commit', async function () { - await Zotero.DB.queryAsync("DROP TABLE IF EXISTS " + tableName); + await Zotero.DB.queryAsync( + "DROP TABLE IF EXISTS " + tableName, false, { noCache: true } + ); Zotero.DB.removeCallback('commit', id); }); } diff --git a/chrome/content/zotero/xpcom/data/dataObjects.js b/chrome/content/zotero/xpcom/data/dataObjects.js index 8cf6d1d50c..c6ca7bc4d4 100644 --- a/chrome/content/zotero/xpcom/data/dataObjects.js +++ b/chrome/content/zotero/xpcom/data/dataObjects.js @@ -569,6 +569,7 @@ Zotero.DataObjects.prototype._loadPrimaryData = Zotero.Promise.coroutine(functio sql, params, { + noCache: true, onRow: function (row) { var id = row.getResultByName(this._ZDO_id); var columns = Object.keys(this._primaryDataSQLParts); @@ -682,7 +683,7 @@ Zotero.DataObjects.prototype._loadRelations = Zotero.Promise.coroutine(function* sql, params, { - noCache: ids.length != 1, + noCache: true, onRow: function (row) { let id = row.getResultByIndex(0); diff --git a/chrome/content/zotero/xpcom/data/items.js b/chrome/content/zotero/xpcom/data/items.js index 365d52aa55..05749b0f89 100644 --- a/chrome/content/zotero/xpcom/data/items.js +++ b/chrome/content/zotero/xpcom/data/items.js @@ -217,7 +217,7 @@ Zotero.Items = function() { sql, params, { - noCache: ids.length != 1, + noCache: true, onRow: function (row) { let itemID = row.getResultByIndex(0); let fieldID = row.getResultByIndex(1); @@ -251,7 +251,7 @@ Zotero.Items = function() { sql, params, { - noCache: ids.length != 1, + noCache: true, onRow: function (row) { let itemID = row.getResultByIndex(0); let item = this._objectCache[itemID]; @@ -283,6 +283,7 @@ Zotero.Items = function() { sql, params, { + noCache: true, onRow: function (row) { let itemID = row.getResultByIndex(0); let title = row.getResultByIndex(1); @@ -334,7 +335,7 @@ Zotero.Items = function() { + 'FROM items LEFT JOIN itemCreators USING (itemID) ' + 'WHERE libraryID=?' + idSQL + " ORDER BY itemID, orderIndex"; var params = [libraryID]; - var rows = yield Zotero.DB.queryAsync(sql, params); + var rows = yield Zotero.DB.queryAsync(sql, params, { noCache: true }); // Mark creator indexes above the number of creators as changed, // so that they're cleared if the item is saved @@ -413,7 +414,7 @@ Zotero.Items = function() { sql, params, { - noCache: ids.length != 1, + noCache: true, onRow: function (row) { let itemID = row.getResultByIndex(0); let item = this._objectCache[itemID]; @@ -476,7 +477,7 @@ Zotero.Items = function() { sql, params, { - noCache: ids.length != 1, + noCache: true, onRow: function (row) { let itemID = row.getResultByIndex(0); let item = this._objectCache[itemID]; @@ -503,7 +504,7 @@ Zotero.Items = function() { sql, params, { - noCache: ids.length != 1, + noCache: true, onRow: function (row) { let itemID = row.getResultByIndex(0); @@ -555,7 +556,7 @@ Zotero.Items = function() { sql, params, { - noCache: ids.length != 1, + noCache: true, onRow: function (row) { let itemID = row.getResultByIndex(0); @@ -633,7 +634,7 @@ Zotero.Items = function() { sql, params, { - noCache: ids.length != 1, + noCache: true, onRow: function (row) { onRow(row, setAttachmentItem); } @@ -687,7 +688,7 @@ Zotero.Items = function() { sql, params, { - noCache: ids.length != 1, + noCache: true, onRow: function (row) { onRow(row, setNoteItem); } @@ -732,7 +733,7 @@ Zotero.Items = function() { sql, params, { - noCache: ids.length != 1, + noCache: true, onRow: function (row) { onRow(row, setAnnotationItem); } @@ -756,7 +757,7 @@ Zotero.Items = function() { sql, params, { - noCache: ids.length != 1, + noCache: true, onRow: function (row) { var itemID = row.getResultByIndex(0); var item = this._objectCache[itemID]; @@ -798,7 +799,7 @@ Zotero.Items = function() { sql, params, { - noCache: ids.length != 1, + noCache: true, onRow: function (row) { let itemID = row.getResultByIndex(0); @@ -851,7 +852,7 @@ Zotero.Items = function() { sql, params, { - noCache: ids.length != 1, + noCache: true, onRow: function (row) { let itemID = row.getResultByIndex(0); diff --git a/chrome/content/zotero/xpcom/data/search.js b/chrome/content/zotero/xpcom/data/search.js index 3d6ed4ee5d..c7ae8d738b 100644 --- a/chrome/content/zotero/xpcom/data/search.js +++ b/chrome/content/zotero/xpcom/data/search.js @@ -604,9 +604,9 @@ Zotero.Search.prototype.search = Zotero.Promise.coroutine(function* (asTempTable tmpTable = "tmpSearchResults_" + Zotero.randomString(8); var sql = "CREATE TEMPORARY TABLE " + tmpTable + " AS " + (yield this._scope.getSQL()); - yield Zotero.DB.queryAsync(sql, yield this._scope.getSQLParams()); + yield Zotero.DB.queryAsync(sql, yield this._scope.getSQLParams(), { noCache: true }); var sql = "CREATE INDEX " + tmpTable + "_itemID ON " + tmpTable + "(itemID)"; - yield Zotero.DB.queryAsync(sql); + yield Zotero.DB.queryAsync(sql, false, { noCache: true }); } // Search ids in temp table @@ -622,7 +622,7 @@ Zotero.Search.prototype.search = Zotero.Promise.coroutine(function* (asTempTable } sql += ")"; - var res = yield Zotero.DB.valueQueryAsync(sql, this._sqlParams); + var res = yield Zotero.DB.valueQueryAsync(sql, this._sqlParams, { noCache: true }); var ids = res ? res.split(",").map(id => parseInt(id)) : []; /* // DEBUG: Should this be here? @@ -636,7 +636,7 @@ Zotero.Search.prototype.search = Zotero.Promise.coroutine(function* (asTempTable } // Or just run main search else { - var ids = yield Zotero.DB.columnQueryAsync(this._sql, this._sqlParams); + var ids = yield Zotero.DB.columnQueryAsync(this._sql, this._sqlParams, { noCache: true }); } //Zotero.debug('IDs from main search or subsearch: '); @@ -680,9 +680,9 @@ Zotero.Search.prototype.search = Zotero.Promise.coroutine(function* (asTempTable if (this.libraryID) { sql += " AND libraryID=?"; } - let res = yield Zotero.DB.valueQueryAsync(sql, this.libraryID); + let res = yield Zotero.DB.valueQueryAsync(sql, this.libraryID, { noCache: true }); scopeIDs = res ? res.split(",").map(id => parseInt(id)) : []; - yield Zotero.DB.queryAsync("DROP TABLE " + tmpTable); + yield Zotero.DB.queryAsync("DROP TABLE " + tmpTable, false, { noCache: true }); } // In ALL mode, include remaining items from the main search else { @@ -797,7 +797,7 @@ Zotero.Search.prototype.search = Zotero.Promise.coroutine(function* (asTempTable } sql = "SELECT GROUP_CONCAT(itemID) FROM items WHERE itemID IN (" + sql + ")"; - var res = yield Zotero.DB.valueQueryAsync(sql); + var res = yield Zotero.DB.valueQueryAsync(sql, false, { noCache: true }); var parentChildIDs = res ? res.split(",").map(id => parseInt(id)) : []; // Add parents and children to main ids @@ -810,7 +810,7 @@ Zotero.Search.prototype.search = Zotero.Promise.coroutine(function* (asTempTable } finally { if (tmpTable && !asTempTable) { - yield Zotero.DB.queryAsync("DROP TABLE IF EXISTS " + tmpTable); + yield Zotero.DB.queryAsync("DROP TABLE IF EXISTS " + tmpTable, false, { noCache: true }); } } @@ -933,9 +933,15 @@ Zotero.Search.idsToTempTable = Zotero.Promise.coroutine(function* (ids) { else { sql += " (itemID INTEGER PRIMARY KEY)"; } - yield Zotero.DB.queryAsync(sql, false, { debug: false }); + yield Zotero.DB.queryAsync(sql, false, { debug: false, noCache: true }); if (ids.length) { - yield Zotero.DB.queryAsync(`CREATE UNIQUE INDEX ${tmpTable}_itemID ON ${tmpTable}(itemID)`); + yield Zotero.DB.queryAsync( + `CREATE UNIQUE INDEX ${tmpTable}_itemID ON ${tmpTable}(itemID)`, + false, + { + noCache: true + } + ); } return tmpTable; diff --git a/chrome/content/zotero/xpcom/data/tags.js b/chrome/content/zotero/xpcom/data/tags.js index f3d6c58200..028d32bc85 100644 --- a/chrome/content/zotero/xpcom/data/tags.js +++ b/chrome/content/zotero/xpcom/data/tags.js @@ -183,7 +183,7 @@ Zotero.Tags = new function() { // Not a perfect locale sort, but speeds up the sort in the tag selector later without any // discernible performance cost sql += "ORDER BY name COLLATE NOCASE"; - var rows = await Zotero.DB.columnQueryAsync(sql, params); + var rows = await Zotero.DB.columnQueryAsync(sql, params, { noCache: !!tmpTable || !!tagIDs }); return rows.map((row) => { var [tagID, type] = row.split(':'); return this.cleanData({ @@ -257,11 +257,15 @@ Zotero.Tags = new function() { // This is ugly, but it's much faster than doing replaceTag() for each item let sql = 'UPDATE OR REPLACE itemTags SET tagID=?, type=0 ' + 'WHERE tagID=? AND itemID IN (' + placeholders + ')'; - yield Zotero.DB.queryAsync(sql, [newTagID, oldTagID].concat(chunk)); + yield Zotero.DB.queryAsync( + sql, [newTagID, oldTagID].concat(chunk), { noCache: true } + ); sql = 'UPDATE items SET synced=0, clientDateModified=? ' + 'WHERE itemID IN (' + placeholders + ')' - yield Zotero.DB.queryAsync(sql, [Zotero.DB.transactionDateTime].concat(chunk)); + yield Zotero.DB.queryAsync( + sql, [Zotero.DB.transactionDateTime].concat(chunk), { noCache: true } + ); yield Zotero.Items.reload(oldItemIDs, ['primaryData', 'tags'], true); }) @@ -375,7 +379,7 @@ Zotero.Tags = new function() { } sql = "DELETE FROM itemTags WHERE ROWID IN (" + rowIDs.join(", ") + ")"; - yield Zotero.DB.queryAsync(sql); + yield Zotero.DB.queryAsync(sql, false, { noCache: true }); yield this.purge(chunk); @@ -386,7 +390,9 @@ Zotero.Tags = new function() { async function (chunk) { var sql = 'UPDATE items SET synced=0, clientDateModified=? ' + 'WHERE itemID IN (' + Array(chunk.length).fill('?').join(',') + ')'; - await Zotero.DB.queryAsync(sql, [Zotero.DB.transactionDateTime].concat(chunk)); + await Zotero.DB.queryAsync( + sql, [Zotero.DB.transactionDateTime].concat(chunk), { noCache: true } + ); await Zotero.Items.reload(itemIDs, ['primaryData', 'tags'], true); } @@ -469,7 +475,10 @@ Zotero.Tags = new function() { return Zotero.DB.queryAsync( "INSERT OR IGNORE INTO tagDelete VALUES " + Array(chunk.length).fill('(?)').join(', '), - chunk + chunk, + { + noCache: true + } ); } ); diff --git a/chrome/content/zotero/xpcom/db.js b/chrome/content/zotero/xpcom/db.js index 75e7fffe79..e11a7aca86 100644 --- a/chrome/content/zotero/xpcom/db.js +++ b/chrome/content/zotero/xpcom/db.js @@ -704,7 +704,13 @@ Zotero.DBConnection.prototype.valueQueryAsync = Zotero.Promise.coroutine(functio if (Zotero.Debug.enabled) { this.logQuery(sql, params, options); } - let rows = yield conn.executeCached(sql, params); + let rows; + if (options && options.noCache) { + rows = yield conn.execute(sql, params); + } + else { + rows = yield conn.executeCached(sql, params); + } return rows.length ? rows[0].getResultByIndex(0) : false; } catch (e) { @@ -747,7 +753,13 @@ Zotero.DBConnection.prototype.columnQueryAsync = Zotero.Promise.coroutine(functi if (Zotero.Debug.enabled) { this.logQuery(sql, params, options); } - let rows = yield conn.executeCached(sql, params); + let rows; + if (options && options.noCache) { + rows = yield conn.execute(sql, params); + } + else { + rows = yield conn.executeCached(sql, params); + } var column = []; for (let i=0, len=rows.length; i x[0]); +}; + + // TEMP Zotero.DBConnection.prototype.vacuum = function () { return this.executeTransaction(function* () {}, { vacuumOnCommit: true }); @@ -905,6 +927,17 @@ Zotero.DBConnection.prototype.isCorruptionError = function (e) { */ Zotero.DBConnection.prototype.closeDatabase = Zotero.Promise.coroutine(function* (permanent) { if (this._connection) { + // TODO: Replace with automatic detection of likely improperly cached statements + // (multiple similar statements, "tmp_", embedded ids) + if (Zotero.isSourceBuild) { + try { + Zotero.debug("Cached DB statements: " + this.numCachedStatements()); + } + catch (e) { + Zotero.logError(e, 1); + } + } + Zotero.debug("Closing database"); this.closed = true; yield this._connection.close();