From 730fd9e44f62346f0a71a40081adf0cc961e5d38 Mon Sep 17 00:00:00 2001 From: Aaron Vaage Date: Wed, 10 May 2017 11:53:31 -0700 Subject: [PATCH] Replacing Cursor Delete In DBEngine After looking into the speeds for removing data from IndexedDB, it was found that using a cursor was slower than the other methods. This change takes the remove logic and changes it to remove each key as part of one transaction. Closes #756 Change-Id: Iec3916650d8a4fe2b6353b604c070d8a0af7426c --- lib/offline/db_engine.js | 176 ++++++++++++++--------------- lib/offline/download_manager.js | 7 +- lib/offline/i_storage_engine.js | 10 +- lib/offline/storage.js | 14 +-- test/offline/db_engine_unit.js | 5 +- test/test/util/memory_db_engine.js | 11 +- 6 files changed, 110 insertions(+), 113 deletions(-) diff --git a/lib/offline/db_engine.js b/lib/offline/db_engine.js index 07c1f2c41b..31c0230a48 100644 --- a/lib/offline/db_engine.js +++ b/lib/offline/db_engine.js @@ -150,61 +150,53 @@ shaka.offline.DBEngine.prototype.destroy = function() { /** @override */ shaka.offline.DBEngine.prototype.get = function(storeName, key) { - return this.createOperation_(storeName, 'readonly', function(store) { - return store.get(key); - }); + var request; + return this.createTransaction_(storeName, 'readonly', function(store) { + request = store.get(key); + }).then(function() { return request.result; }); }; /** @override */ shaka.offline.DBEngine.prototype.forEach = function(storeName, callback) { - return this.createOperation_(storeName, 'readonly', function(store) { - return store.openCursor(); - }, function(/** IDBCursor */ cursor) { - if (!cursor) return; - - callback(cursor.value); - cursor.continue(); + return this.createTransaction_(storeName, 'readonly', function(store) { + var request = store.openCursor(); + request.onsuccess = function(event) { + var cursor = event.target.result; + if (cursor) { + callback(cursor.value); + cursor.continue(); + } + }; }); }; /** @override */ shaka.offline.DBEngine.prototype.insert = function(storeName, value) { - return this.createOperation_(storeName, 'readwrite', function(store) { - return store.put(value); + return this.createTransaction_(storeName, 'readwrite', function(store) { + store.put(value); }); }; /** @override */ shaka.offline.DBEngine.prototype.remove = function(storeName, key) { - return this.createOperation_(storeName, 'readwrite', function(store) { - return store.delete(key); + return this.createTransaction_(storeName, 'readwrite', function(store) { + store.delete(key); }); }; /** @override */ -shaka.offline.DBEngine.prototype.removeWhere = function(storeName, callback) { - var async = []; - return this.createOperation_(storeName, 'readwrite', function(store) { - return store.openCursor(); - }, function(/** IDBCursor */ cursor) { - if (!cursor) return; - - if (callback(cursor.value)) { - var request = cursor.delete(); - var p = new shaka.util.PublicPromise(); - request.onsuccess = p.resolve; - request.onerror = shaka.offline.DBEngine.onError_.bind(null, request, p); - async.push(p); +shaka.offline.DBEngine.prototype.removeKeys = function(storeName, + keys, + opt_onKeyRemoved) { + return this.createTransaction_(storeName, 'readwrite', function(store) { + for (var i = 0; i < keys.length; i++) { + var request = store.delete(keys[i]); + request.onsuccess = opt_onKeyRemoved || function(event) { }; } - cursor.continue(); - }).then(function() { - return Promise.all(async); - }).then(function() { - return async.length; }); }; @@ -225,75 +217,85 @@ shaka.offline.DBEngine.prototype.reserveId = function(storeName) { * @private */ shaka.offline.DBEngine.prototype.getNextId_ = function(storeName) { - var ret = 0; - return this.createOperation_(storeName, 'readonly', function(store) { - return store.openCursor(null, 'prev'); - }, function(/** IDBCursor */ cursor) { - if (cursor) - ret = cursor.key + 1; - }).then(function() { return ret; }); + var id = 0; + return this.createTransaction_(storeName, 'readonly', function(store) { + var request = store.openCursor(null, 'prev'); + request.onsuccess = function(event) { + var cursor = event.target.result; + if (cursor) { + id = cursor.key + 1; + } + }; + }).then(function() { return id; }); }; /** - * Creates a new transaction for the given store name and calls the given - * callback to create a request. It then wraps the given request in an - * operation and returns the resulting promise. The Promise resolves when - * the transaction is complete, which will be after opt_success is called. + * Creates a new transaction for the given store name and calls |action| to + * modify the store. The transaction will resolve or reject the promise + * returned by this function. * * @param {string} storeName * @param {string} type - * @param {function(!IDBObjectStore):!IDBRequest} createRequest - * @param {(function(*))=} opt_success The value of onsuccess for the request. + * @param {!function(IDBObjectStore)} action + * * @return {!Promise} * @private */ -shaka.offline.DBEngine.prototype.createOperation_ = function( - storeName, type, createRequest, opt_success) { +shaka.offline.DBEngine.prototype.createTransaction_ = function(storeName, + type, + action) { + goog.asserts.assert(this.db_, 'Must not be destroyed'); goog.asserts.assert(type == 'readonly' || type == 'readwrite', 'Type must be "readonly" or "readwrite"'); - var trans = this.db_.transaction([storeName], type); - var request = createRequest(trans.objectStore(storeName)); - - var p = new shaka.util.PublicPromise(); - if (opt_success) - request.onsuccess = function(event) { opt_success(event.target.result); }; - request.onerror = function(event) { - shaka.log.info('The request unexpectedly failed', - request.error ? request.error.message : 'unknown error'); + var op = { + transaction: this.db_.transaction([storeName], type), + promise: new shaka.util.PublicPromise() }; - var op = {transaction: trans, promise: p}; - this.operations_.push(op); - - // Only remove the transaction once it has completed, which may be after the - // request is complete (e.g. it may need to write to disk). - var removeOp = (function() { - var i = this.operations_.indexOf(op); - goog.asserts.assert(i >= 0, 'Operation must be in the list.'); - this.operations_.splice(i, 1); + op.transaction.oncomplete = (function(event) { + this.closeOperation_(op); + op.promise.resolve(); }.bind(this)); - trans.oncomplete = function(event) { - removeOp(); - p.resolve(request.result); - }; - trans.onerror = function(event) { - // Otherwise Firefox will raise an error which will cause a karma failure. - // This will not stop the onabort callback from firing. - event.preventDefault(); - }; + // We will see an onabort call via: // 1. request error -> transaction error -> transaction abort // 2. transaction commit fail -> transaction abort // As any transaction error will result in an abort, it is better to listen // for an abort so that we will catch all failed transaction operations. - trans.onabort = function(event) { - removeOp(); - shaka.offline.DBEngine.onError_(request, p, event); - }; - return p; + op.transaction.onabort = (function(event) { + this.closeOperation_(op); + shaka.offline.DBEngine.onError_(op.transaction, op.promise, event); + }.bind(this)); + + // We need to prevent default on the onerror event or else Firefox will + // raise an error which will cause a karma failure. This will not stop the + // onabort callback from firing. + op.transaction.onerror = (function(event) { + event.preventDefault(); + }.bind(this)); + + var store = op.transaction.objectStore(storeName); + action(store); + + this.operations_.push(op); + + return op.promise; +}; + + +/** + * Close an open operation. + * + * @param {!shaka.offline.DBEngine.Operation} op + * @private + */ +shaka.offline.DBEngine.prototype.closeOperation_ = function(op) { + var i = this.operations_.indexOf(op); + goog.asserts.assert(i >= 0, 'Operation must be in the list.'); + this.operations_.splice(i, 1); }; @@ -357,27 +359,25 @@ shaka.offline.DBEngine.prototype.createConnection_ = function( /** - * Rejects the given Promise with an unknown error. + * Rejects the given Promise using the error fromt the transaction. * - * @param {!IDBRequest} request + * @param {!IDBTransaction|!IDBRequest} errorSource * @param {!shaka.util.PublicPromise} promise * @param {Event} event * @private */ -shaka.offline.DBEngine.onError_ = function(request, promise, event) { - // |request.error| can be null as per 3.3.4 point 2 of the W3 spec. When a - // request does not have an error and is passed here it is because it was from - // |transaction.onabort| and therefore not an actual error. - if (!request.error || request.error.name == 'AbortError') { +shaka.offline.DBEngine.onError_ = function(errorSource, promise, event) { + + if (errorSource.error) { promise.reject(new shaka.util.Error( shaka.util.Error.Severity.CRITICAL, shaka.util.Error.Category.STORAGE, - shaka.util.Error.Code.OPERATION_ABORTED)); + shaka.util.Error.Code.INDEXED_DB_ERROR, errorSource.error)); } else { promise.reject(new shaka.util.Error( shaka.util.Error.Severity.CRITICAL, shaka.util.Error.Category.STORAGE, - shaka.util.Error.Code.INDEXED_DB_ERROR, request.error)); + shaka.util.Error.Code.OPERATION_ABORTED)); } // Firefox will raise an error which will cause a karma failure. diff --git a/lib/offline/download_manager.js b/lib/offline/download_manager.js index 252fde02e9..80f9ad3909 100644 --- a/lib/offline/download_manager.js +++ b/lib/offline/download_manager.js @@ -127,11 +127,8 @@ shaka.offline.DownloadManager.prototype.destroy = function() { var storage = this.storageEngine_; var segments = this.storedSegments_; var p = this.promise_ || Promise.resolve(); - p = p.then(function() { - return Promise.all(segments.map(function(id) { - return storage.remove('segment', id); - })); - }); + p = p.then(function() { return storage.removeKeys('segment', segments); }); + // Don't destroy() storageEngine since it is owned by Storage. this.segments_ = {}; diff --git a/lib/offline/i_storage_engine.js b/lib/offline/i_storage_engine.js index 088522fe66..1232343f09 100644 --- a/lib/offline/i_storage_engine.js +++ b/lib/offline/i_storage_engine.js @@ -101,14 +101,14 @@ shaka.offline.IStorageEngine.prototype.remove; /** - * Removes all items for which the given predicate returns true. + * Removes the elements with the given keys. * * @param {string} storeName - * @param {function(T):boolean} callback - * @return {!Promise.} - * @template T + * @param {!Array} keys + * @param {function()=} opt_onKeyRemoved + * @return {!Promise} */ -shaka.offline.IStorageEngine.prototype.removeWhere; +shaka.offline.IStorageEngine.prototype.removeKeys; /** diff --git a/lib/offline/storage.js b/lib/offline/storage.js index b9304cc374..0e268de0da 100644 --- a/lib/offline/storage.js +++ b/lib/offline/storage.js @@ -321,14 +321,12 @@ shaka.offline.Storage.prototype.remove = function(content) { var deleteCount = 0; var segmentCount = segments.length; var callback = this.config_.progressCallback; - return this.storageEngine_.removeWhere('segment', function(segment) { - var i = segments.indexOf(segment.key); - if (i >= 0) { - callback(content, deleteCount / segmentCount); - deleteCount++; - } - return i >= 0; - }.bind(this)); + + return this.storageEngine_.removeKeys('segment', segments, function() { + deleteCount++; + callback(content, deleteCount / segmentCount); + }); + }.bind(this)).then(function() { this.checkDestroyed_(); this.config_.progressCallback(content, 1); diff --git a/test/offline/db_engine_unit.js b/test/offline/db_engine_unit.js index 25b56c6e01..8020ec9761 100644 --- a/test/offline/db_engine_unit.js +++ b/test/offline/db_engine_unit.js @@ -113,7 +113,7 @@ describe('DBEngine', function() { return db.get('test', 2); }).then(function(data) { expect(data).toBeFalsy(); - return db.removeWhere('test', function(s) { return s.i >= 7; }); + return db.removeKeys('test', [4, 5, 6]); }).then(function() { return db.get('test', 5); }).then(function(data) { @@ -221,12 +221,11 @@ describe('DBEngine', function() { // the transaction will abort. This should cause the promise to be // rejected. db.insert = function(storeName, value) { - return this.createOperation_(storeName, 'readwrite', function(store) { + return this.createTransaction_(storeName, 'readwrite', function(store) { var request = store.put(value); request.onsuccess = function(event) { request.transaction.abort(); }; - return request; }); }; diff --git a/test/test/util/memory_db_engine.js b/test/test/util/memory_db_engine.js index c094e7519b..7f2118fab4 100644 --- a/test/test/util/memory_db_engine.js +++ b/test/test/util/memory_db_engine.js @@ -105,12 +105,15 @@ shaka.test.MemoryDBEngine.prototype.remove = function(storeName, key) { /** @override */ -shaka.test.MemoryDBEngine.prototype.removeWhere = function( - storeName, predicate) { +shaka.test.MemoryDBEngine.prototype.removeKeys = function(storeName, + keys, + opt_onKeyRemoved) { var store = this.getStore_(storeName); for (var key in store) { - if (predicate(store[Number(key)])) - delete store[Number(key)]; + key = Number(key); + if (keys.indexOf(key) >= 0) { + delete store[key]; + } } return Promise.resolve(); };