From 09c3a95a7e17f88972f1e616f6a90e4b8d2a2531 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Thu, 5 May 2016 04:34:19 -0400 Subject: [PATCH] Improve downloaded object processing - Use an increasing notifier batch size, so objects initially appear one by one but then start showing up in batches, up to 50. (UI updates are expensive, so for larger syncs we don't want to update after each object.) - Avoid separate save to update attachment file sync state, which was also happening outside of notifier batches (causing individual updates regardless of the batch size) - Add a 10ms delay after processing each object, which keeps the UI responsive during downloads. #989 could reduce this to 1 during idle, to save a few minutes when downloading very large libraries. --- .../content/zotero/xpcom/sync/syncEngine.js | 37 ++++++++-- chrome/content/zotero/xpcom/sync/syncLocal.js | 70 ++++++++++--------- 2 files changed, 69 insertions(+), 38 deletions(-) diff --git a/chrome/content/zotero/xpcom/sync/syncEngine.js b/chrome/content/zotero/xpcom/sync/syncEngine.js index d739747d9..d1d64cff0 100644 --- a/chrome/content/zotero/xpcom/sync/syncEngine.js +++ b/chrome/content/zotero/xpcom/sync/syncEngine.js @@ -591,8 +591,9 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu ); var conflicts = []; + var num = 0; - // Process batches as soon as they're available + // Process batches when they're available, one at a time yield Zotero.Promise.map( json, function (batch) { @@ -618,9 +619,31 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu objectType, this.libraryID, batch, - this._getOptions() + this._getOptions({ + onObjectProcessed: () => { + num++; + }, + // Increase the notifier batch size as we go + getNotifierBatchSize: () => { + var size; + if (num < 10) { + size = 1; + } + else if (num < 50) { + size = 5; + } + else if (num < 150) { + size = 25; + } + else { + size = 50; + } + return Math.min(size, batch.length); + } + }) ) .then(function (results) { + num += results.length; let processedKeys = []; let conflictResults = []; results.forEach(x => { @@ -639,7 +662,10 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu keys = Zotero.Utilities.arrayDiff(keys, processedKeys); conflicts.push(...conflictResults); }.bind(this)); - }.bind(this) + }.bind(this), + { + concurrency: 1 + } ); if (!keys.length || keys.length == lastLength) { @@ -1433,9 +1459,12 @@ Zotero.Sync.Data.Engine.prototype._fullSync = Zotero.Promise.coroutine(function* }); -Zotero.Sync.Data.Engine.prototype._getOptions = function () { +Zotero.Sync.Data.Engine.prototype._getOptions = function (additionalOpts = {}) { var options = {}; this.optionNames.forEach(x => options[x] = this[x]); + for (let opt in additionalOpts) { + options[opt] = additionalOpts[opt]; + } return options; } diff --git a/chrome/content/zotero/xpcom/sync/syncLocal.js b/chrome/content/zotero/xpcom/sync/syncLocal.js index adc747128..3c99c538e 100644 --- a/chrome/content/zotero/xpcom/sync/syncLocal.js +++ b/chrome/content/zotero/xpcom/sync/syncLocal.js @@ -535,14 +535,19 @@ Zotero.Sync.Data.Local = { }); } - var batchSize = 10; + var batchSize = options.getNotifierBatchSize ? options.getNotifierBatchSize() : json.length; var notifierQueues = []; + try { for (let i = 0; i < json.length; i++) { // Batch notifier updates if (notifierQueues.length == batchSize) { yield Zotero.Notifier.commit(notifierQueues); notifierQueues = []; + // Get the current batch size, which might have increased + if (options.getNotifierBatchSize) { + batchSize = options.getNotifierBatchSize() + } } let notifierQueue = new Zotero.Notifier.Queue; @@ -786,6 +791,13 @@ Zotero.Sync.Data.Local = { throw e; } } + finally { + if (options.onObjectProcessed) { + options.onObjectProcessed(); + } + } + + yield Zotero.Promise.delay(10); } } finally { @@ -825,32 +837,10 @@ Zotero.Sync.Data.Local = { }, - _onObjectProcessed: Zotero.Promise.coroutine(function* (obj, jsonObject, isNewObject, storageDetailsChanged) { - var jsonData = jsonObject.data; - - // Delete older versions of the object in the cache - yield this.deleteCacheObjectVersions( - obj.objectType, obj.libraryID, jsonData.key, null, jsonData.version - 1 - ); - - // Delete from sync queue - yield this._removeObjectFromSyncQueue(obj.objectType, obj.libraryID, jsonData.key); - - // Mark updated attachments for download - if (obj.objectType == 'item' && obj.isImportedAttachment()) { - // If storage changes were made (attachment mtime or hash), mark - // library as requiring download - if (isNewObject || storageDetailsChanged) { - Zotero.Libraries.get(obj.libraryID).storageDownloadNeeded = true; - } - - yield this._checkAttachmentForDownload( - obj, jsonData.mtime, isNewObject - ); - } - }), - - + /** + * Check whether an attachment's file mod time matches the given mod time, and mark the file + * for download if not (or if this is a new attachment) + */ _checkAttachmentForDownload: Zotero.Promise.coroutine(function* (item, mtime, isNewObject) { var markToDownload = false; if (!isNewObject) { @@ -882,7 +872,6 @@ Zotero.Sync.Data.Local = { } if (markToDownload) { item.attachmentSyncState = "to_download"; - yield item.save({ skipAll: true }); } }), @@ -956,7 +945,7 @@ Zotero.Sync.Data.Local = { Zotero.debug("Processing resolved conflicts"); - let batchSize = 50; + let batchSize = mergeData.length; let notifierQueues = []; try { for (let i = 0; i < mergeData.length; i++) { @@ -1125,6 +1114,9 @@ Zotero.Sync.Data.Local = { if (!options.skipData) { obj.fromJSON(json.data); } + if (obj.objectType == 'item' && obj.isImportedAttachment()) { + this._checkAttachmentForDownload(obj, json.data.mtime, options.isNewObject); + } obj.version = json.data.version; if (!options.saveAsChanged) { obj.synced = true; @@ -1143,12 +1135,22 @@ Zotero.Sync.Data.Local = { yield this.saveCacheObject(obj.objectType, obj.libraryID, json.data); results.processed = true; - yield this._onObjectProcessed( - obj, - json, - options.isNewObject, - options.storageDetailsChanged + // Delete older versions of the object in the cache + yield this.deleteCacheObjectVersions( + obj.objectType, obj.libraryID, json.key, null, json.version - 1 ); + + // Delete from sync queue + yield this._removeObjectFromSyncQueue(obj.objectType, obj.libraryID, json.key); + + // Mark updated attachments for download + if (obj.objectType == 'item' && obj.isImportedAttachment()) { + // If storage changes were made (attachment mtime or hash), mark + // library as requiring download + if (options.isNewObject || options.storageDetailsChanged) { + Zotero.Libraries.get(obj.libraryID).storageDownloadNeeded = true; + } + } } catch (e) { // For now, allow sync to proceed after all errors