From 17e23ffb33e11dfa26830e3fd58d0c57b3a86ef1 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Tue, 9 Jun 2020 11:46:56 +0200 Subject: [PATCH 1/4] Convert the `ChunkedStreamManager.chunksNeededByRequest` property to a `Map` (containing `Set`s) Compared to regular `Object`s, `Map`s (and `Set`s) have a number of advantageous properties: Of particular importance in this case is the built-in iteration support, and that determining if the structure is empty is easy. --- src/core/chunked_stream.js | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/core/chunked_stream.js b/src/core/chunked_stream.js index 2ba358fe9..dfc8920d9 100644 --- a/src/core/chunked_stream.js +++ b/src/core/chunked_stream.js @@ -319,7 +319,7 @@ class ChunkedStreamManager { this.currRequestId = 0; - this.chunksNeededByRequest = Object.create(null); + this._chunksNeededByRequest = new Map(); this.requestsByChunk = Object.create(null); this.promisesByRequest = Object.create(null); this.progressiveDataLength = 0; @@ -384,15 +384,15 @@ class ChunkedStreamManager { _requestChunks(chunks) { const requestId = this.currRequestId++; - const chunksNeeded = Object.create(null); - this.chunksNeededByRequest[requestId] = chunksNeeded; + const chunksNeeded = new Set(); + this._chunksNeededByRequest.set(requestId, chunksNeeded); for (const chunk of chunks) { if (!this.stream.hasChunk(chunk)) { - chunksNeeded[chunk] = true; + chunksNeeded.add(chunk); } } - if (isEmptyObj(chunksNeeded)) { + if (chunksNeeded.size === 0) { return Promise.resolve(); } @@ -400,8 +400,7 @@ class ChunkedStreamManager { this.promisesByRequest[requestId] = capability; const chunksToRequest = []; - for (let chunk in chunksNeeded) { - chunk = chunk | 0; + for (const chunk of chunksNeeded) { if (!(chunk in this.requestsByChunk)) { this.requestsByChunk[chunk] = []; chunksToRequest.push(chunk); @@ -526,12 +525,12 @@ class ChunkedStreamManager { delete this.requestsByChunk[curChunk]; for (const requestId of requestIds) { - const chunksNeeded = this.chunksNeededByRequest[requestId]; - if (curChunk in chunksNeeded) { - delete chunksNeeded[curChunk]; + const chunksNeeded = this._chunksNeededByRequest.get(requestId); + if (chunksNeeded.has(curChunk)) { + chunksNeeded.delete(curChunk); } - if (!isEmptyObj(chunksNeeded)) { + if (chunksNeeded.size > 0) { continue; } loadedRequests.push(requestId); From dda7a5d1b7888287cf7bb82995190d5f2cbbff7b Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Tue, 9 Jun 2020 16:06:26 +0200 Subject: [PATCH 2/4] Convert the `ChunkedStreamManager.requestsByChunk` property to a `Map` Compared to regular `Object`s, `Map`s have a number of advantageous properties: Of particular importance in this case is the built-in iteration support, and that determining if the structure is empty is easy. --- src/core/chunked_stream.js | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/core/chunked_stream.js b/src/core/chunked_stream.js index dfc8920d9..7437ae391 100644 --- a/src/core/chunked_stream.js +++ b/src/core/chunked_stream.js @@ -320,7 +320,7 @@ class ChunkedStreamManager { this.currRequestId = 0; this._chunksNeededByRequest = new Map(); - this.requestsByChunk = Object.create(null); + this._requestsByChunk = new Map(); this.promisesByRequest = Object.create(null); this.progressiveDataLength = 0; this.aborted = false; @@ -401,11 +401,14 @@ class ChunkedStreamManager { const chunksToRequest = []; for (const chunk of chunksNeeded) { - if (!(chunk in this.requestsByChunk)) { - this.requestsByChunk[chunk] = []; + let requestIds = this._requestsByChunk.get(chunk); + if (!requestIds) { + requestIds = []; + this._requestsByChunk.set(chunk, requestIds); + chunksToRequest.push(chunk); } - this.requestsByChunk[chunk].push(requestId); + requestIds.push(requestId); } if (!chunksToRequest.length) { @@ -521,8 +524,11 @@ class ChunkedStreamManager { const loadedRequests = []; for (let curChunk = beginChunk; curChunk < endChunk; ++curChunk) { // The server might return more chunks than requested. - const requestIds = this.requestsByChunk[curChunk] || []; - delete this.requestsByChunk[curChunk]; + const requestIds = this._requestsByChunk.get(curChunk); + if (!requestIds) { + continue; + } + this._requestsByChunk.delete(curChunk); for (const requestId of requestIds) { const chunksNeeded = this._chunksNeededByRequest.get(requestId); @@ -539,7 +545,7 @@ class ChunkedStreamManager { // If there are no pending requests, automatically fetch the next // unfetched chunk of the PDF file. - if (!this.disableAutoFetch && isEmptyObj(this.requestsByChunk)) { + if (!this.disableAutoFetch && this._requestsByChunk.size === 0) { let nextEmptyChunk; if (this.stream.numChunksLoaded === 1) { // This is a special optimization so that after fetching the first From 159e13c4e45b01b267e783cd2ba5e7e25cef33d9 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Tue, 9 Jun 2020 16:25:41 +0200 Subject: [PATCH 3/4] Convert the `ChunkedStreamManager.promisesByRequest` property to a `Map` Compared to regular `Object`s, `Map`s have a number of advantageous properties: Of particular importance in this case is the built-in iteration support, and that determining if the structure is empty is easy. --- src/core/chunked_stream.js | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/core/chunked_stream.js b/src/core/chunked_stream.js index 7437ae391..f041f8bc0 100644 --- a/src/core/chunked_stream.js +++ b/src/core/chunked_stream.js @@ -18,7 +18,6 @@ import { arrayByteLength, arraysToBytes, createPromiseCapability, - isEmptyObj, } from "../shared/util.js"; import { MissingDataException } from "./core_utils.js"; @@ -321,7 +320,7 @@ class ChunkedStreamManager { this._chunksNeededByRequest = new Map(); this._requestsByChunk = new Map(); - this.promisesByRequest = Object.create(null); + this._promisesByRequest = new Map(); this.progressiveDataLength = 0; this.aborted = false; @@ -397,7 +396,7 @@ class ChunkedStreamManager { } const capability = createPromiseCapability(); - this.promisesByRequest[requestId] = capability; + this._promisesByRequest.set(requestId, capability); const chunksToRequest = []; for (const chunk of chunksNeeded) { @@ -564,8 +563,8 @@ class ChunkedStreamManager { } for (const requestId of loadedRequests) { - const capability = this.promisesByRequest[requestId]; - delete this.promisesByRequest[requestId]; + const capability = this._promisesByRequest.get(requestId); + this._promisesByRequest.delete(requestId); capability.resolve(); } @@ -592,8 +591,8 @@ class ChunkedStreamManager { if (this.pdfNetworkStream) { this.pdfNetworkStream.cancelAllRequests(reason); } - for (const requestId in this.promisesByRequest) { - this.promisesByRequest[requestId].reject(reason); + for (const [, capability] of this._promisesByRequest) { + capability.reject(reason); } } } From 88fdb482b097b7d899215838f657352deb4e1199 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Tue, 9 Jun 2020 16:48:03 +0200 Subject: [PATCH 4/4] Move the `isEmptyObj` helper function from `src/shared/util.js` to `test/unit/test_utils.js` Since this helper function is no longer used anywhere in the main code-base, but only in a couple of unit-tests, it's thus being moved to a more appropriate spot. Finally, the implementation of `isEmptyObj` is also tweaked slightly by removing the manual loop. --- src/shared/util.js | 8 -------- test/unit/metadata_spec.js | 2 +- test/unit/test_utils.js | 9 +++++++++ test/unit/util_spec.js | 11 ----------- 4 files changed, 10 insertions(+), 20 deletions(-) diff --git a/src/shared/util.js b/src/shared/util.js index eeb666d20..64b07f526 100644 --- a/src/shared/util.js +++ b/src/shared/util.js @@ -793,13 +793,6 @@ function utf8StringToString(str) { return unescape(encodeURIComponent(str)); } -function isEmptyObj(obj) { - for (const key in obj) { - return false; - } - return true; -} - function isBool(v) { return typeof v === "boolean"; } @@ -931,7 +924,6 @@ export { isArrayBuffer, isArrayEqual, isBool, - isEmptyObj, isNum, isString, isSameOrigin, diff --git a/test/unit/metadata_spec.js b/test/unit/metadata_spec.js index 710a8fdb7..02eabd271 100644 --- a/test/unit/metadata_spec.js +++ b/test/unit/metadata_spec.js @@ -13,7 +13,7 @@ * limitations under the License. */ -import { isEmptyObj } from "../../src/shared/util.js"; +import { isEmptyObj } from "./test_utils.js"; import { Metadata } from "../../src/display/metadata.js"; describe("metadata", function () { diff --git a/test/unit/test_utils.js b/test/unit/test_utils.js index 09d2827a2..a1156c5a8 100644 --- a/test/unit/test_utils.js +++ b/test/unit/test_utils.js @@ -175,6 +175,14 @@ function createIdFactory(pageIndex) { return page.idFactory; } +function isEmptyObj(obj) { + assert( + typeof obj === "object" && obj !== null, + "isEmptyObj - invalid argument." + ); + return Object.keys(obj).length === 0; +} + export { DOMFileReaderFactory, NodeFileReaderFactory, @@ -184,4 +192,5 @@ export { buildGetDocumentParams, TEST_PDFS_PATH, createIdFactory, + isEmptyObj, }; diff --git a/test/unit/util_spec.js b/test/unit/util_spec.js index eca89e41f..8d5a76968 100644 --- a/test/unit/util_spec.js +++ b/test/unit/util_spec.js @@ -19,7 +19,6 @@ import { createValidAbsoluteUrl, isArrayBuffer, isBool, - isEmptyObj, isNum, isSameOrigin, isString, @@ -89,16 +88,6 @@ describe("util", function () { }); }); - describe("isEmptyObj", function () { - it("handles empty objects", function () { - expect(isEmptyObj({})).toEqual(true); - }); - - it("handles non-empty objects", function () { - expect(isEmptyObj({ foo: "bar" })).toEqual(false); - }); - }); - describe("isNum", function () { it("handles numeric values", function () { expect(isNum(1)).toEqual(true);