From 7212ff4eeac2696b2733bb8de09dbb5e0715d8bc Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 5 Sep 2019 10:08:54 +0200 Subject: [PATCH 1/3] Stop checking for the `response` property, on `XMLHttpRequest`, when setting up the `WorkerMessageHandler` This check was added in PR 2445, however it's no longer necessary since all data[1] is now loaded on the main-thread (and then transferred to the worker-thread). Furthermore, by default the Fetch API is now (usually) used rather than `XMLHttpRequest`. All in all, while these checks *were* necessary at one point that's no longer the case and they can thus be removed. --- [1] This includes both the actual PDF data, as well as the CMap data. --- src/core/worker.js | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/src/core/worker.js b/src/core/worker.js index 4da105f89ac89..17a44d88d4e0a 100644 --- a/src/core/worker.js +++ b/src/core/worker.js @@ -73,19 +73,7 @@ var WorkerMessageHandler = { // making sure postMessage transfers are working var supportTransfers = data[0] === 255; handler.postMessageTransfers = supportTransfers; - // check if the response property is supported by xhr - var xhr = new XMLHttpRequest(); - var responseExists = 'response' in xhr; - // check if the property is actually implemented - try { - xhr.responseType; // eslint-disable-line no-unused-expressions - } catch (e) { - responseExists = false; - } - if (!responseExists) { - handler.send('test', false); - return; - } + handler.send('test', { supportTypedArray: true, supportTransfers, From f0534b9b51fe3764d144e1a867b0b2645535caa9 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 5 Sep 2019 10:30:09 +0200 Subject: [PATCH 2/3] Adjust the values sent, with the 'test' message, by the `WorkerMessageHandler.setup` method Note how the sent values have inconsistent types, with a boolean in one case and an object in the other (normal) case. Furthermore, explicitly sending a `supportTypedArray: true` property seems superfluous at least to me. --- src/core/worker.js | 9 +++------ src/display/api.js | 2 +- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/core/worker.js b/src/core/worker.js index 17a44d88d4e0a..848b6f7e39e76 100644 --- a/src/core/worker.js +++ b/src/core/worker.js @@ -67,17 +67,14 @@ var WorkerMessageHandler = { // check if Uint8Array can be sent to worker if (!(data instanceof Uint8Array)) { - handler.send('test', false); + handler.send('test', null); return; } // making sure postMessage transfers are working - var supportTransfers = data[0] === 255; + const supportTransfers = data[0] === 255; handler.postMessageTransfers = supportTransfers; - handler.send('test', { - supportTypedArray: true, - supportTransfers, - }); + handler.send('test', { supportTransfers, }); }); handler.on('configure', function wphConfigure(data) { diff --git a/src/display/api.js b/src/display/api.js index a964d52ff119b..fa41abab7349e 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -1665,7 +1665,7 @@ const PDFWorker = (function PDFWorkerClosure() { terminateEarly(); return; // worker was destroyed } - if (data && data.supportTypedArray) { + if (data) { // supportTypedArray this._messageHandler = messageHandler; this._port = worker; this._webWorker = worker; From 7dea3f9389be0a8ea5e153a9695f67184e85b7c4 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 5 Sep 2019 10:42:30 +0200 Subject: [PATCH 3/3] [api-minor] Remove the `postMessageTransfers` parameter, and thus the ability to manually disable transferring of data, from the API By transfering, rather than copying, `ArrayBuffer`s between the main- and worker-threads, you can avoid unnecessary allocations by only having *one* copy of the same data. Hence manually setting `postMessageTransfers: false`, when calling `getDocument`, is a performance footgun[1] which will do nothing but waste memory. Given that every reasonably modern browser supports `postMessage` transfers[2], I really don't see why it should be possible to force-disable this functionality. Looking at the browser support, for `postMessage` transfers[2], it's highly unlikely that PDF.js is even usable in browsers without it. However, the feature testing of `postMessage` transfers is kept for the time being just to err on the safe side. --- [1] This is somewhat similar to the, now removed, `disableWorker` parameter which also provided API users a much too simple way of reducing performance. [2] See e.g. https://developer.mozilla.org/en-US/docs/Web/API/MessagePort/postMessage#Browser_compatibility and https://developer.mozilla.org/en-US/docs/Web/API/Transferable#Browser_compatibility --- src/display/api.js | 11 +++-------- web/app_options.js | 5 ----- 2 files changed, 3 insertions(+), 13 deletions(-) diff --git a/src/display/api.js b/src/display/api.js index fa41abab7349e..f65b8215d2dfd 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -146,8 +146,6 @@ function setPDFNetworkStreamFactory(pdfNetworkStreamFactory) { * 2^16 = 65536. * @property {PDFWorker} worker - (optional) The worker that will be used for * the loading and parsing of the PDF data. - * @property {boolean} postMessageTransfers - (optional) Enables transfer usage - * in postMessage for ArrayBuffers. The default value is `true`. * @property {number} verbosity - (optional) Controls the logging level; the * constants from {VerbosityLevel} should be used. * @property {string} docBaseUrl - (optional) The base URL of the document, @@ -319,7 +317,6 @@ function getDocument(src) { if (!worker) { const workerParams = { - postMessageTransfers: params.postMessageTransfers, verbosity: params.verbosity, port: GlobalWorkerOptions.workerPort, }; @@ -1476,8 +1473,6 @@ class LoopbackPort { * @typedef {Object} PDFWorkerParameters * @property {string} name - (optional) The name of the worker. * @property {Object} port - (optional) The `workerPort`. - * @property {boolean} postMessageTransfers - (optional) Enables transfer usage - * in postMessage for ArrayBuffers. The default value is `true`. * @property {number} verbosity - (optional) Controls the logging level; the * constants from {VerbosityLevel} should be used. */ @@ -1568,7 +1563,7 @@ const PDFWorker = (function PDFWorkerClosure() { * @param {PDFWorkerParameters} params - The worker initialization parameters. */ class PDFWorker { - constructor({ name = null, port = null, postMessageTransfers = true, + constructor({ name = null, port = null, verbosity = getVerbosityLevel(), } = {}) { if (port && pdfWorkerPorts.has(port)) { throw new Error('Cannot use more than one PDFWorker per port'); @@ -1576,7 +1571,7 @@ const PDFWorker = (function PDFWorkerClosure() { this.name = name; this.destroyed = false; - this.postMessageTransfers = postMessageTransfers !== false; + this.postMessageTransfers = true; this.verbosity = verbosity; this._readyCapability = createPromiseCapability(); @@ -1705,7 +1700,7 @@ const PDFWorker = (function PDFWorkerClosure() { try { messageHandler.send('test', testObj, [testObj.buffer]); } catch (ex) { - info('Cannot use postMessage transfers'); + warn('Cannot use postMessage transfers.'); testObj[0] = 0; messageHandler.send('test', testObj); } diff --git a/web/app_options.js b/web/app_options.js index 5ad60b6d762a2..6e5bd0085aebe 100644 --- a/web/app_options.js +++ b/web/app_options.js @@ -206,11 +206,6 @@ const defaultOptions = { value: false, kind: OptionKind.API, }, - postMessageTransfers: { - /** @type {boolean} */ - value: true, - kind: OptionKind.API, - }, verbosity: { /** @type {number} */ value: 1,