mirrors/zotero
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages 1 Wiki Activity Actions

Avoid memory leaks from caching unique DB statements

Browse source 
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
This commit is contained in:
Dan Stillman 2021-04-21 17:33:24 -04:00
parent 12045a7cf8
commit ea42789206
7 changed files with 86 additions and 34 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
chrome/content/zotero/xpcom/collectionTreeRow.js
View file

@ -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 {

4
chrome/content/zotero/xpcom/collectionTreeView.js
View file

@ -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);
});
}

3
chrome/content/zotero/xpcom/data/dataObjects.js
View file

@ -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);

27
chrome/content/zotero/xpcom/data/items.js
View file

@ -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);

26
chrome/content/zotero/xpcom/data/search.js
View file

@ -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;

21
chrome/content/zotero/xpcom/data/tags.js
View file

@ -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
}
);
}
);

37
chrome/content/zotero/xpcom/db.js
View file

@ -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<len; i++) {
column.push(rows[i].getResultByIndex(0));
@ -864,6 +876,16 @@ Zotero.DBConnection.prototype.observe = function(subject, topic, data) {
}
Zotero.DBConnection.prototype.numCachedStatements = function () {
return this._connection._connectionData._cachedStatements.size;
};
Zotero.DBConnection.prototype.getCachedStatements = function () {
return [...this._connection._connectionData._cachedStatements].map(x => 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();