From a598596978fb7841ad9ec577bef227b8c9acbf84 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 5 Sep 2019 10:42:30 +0200 Subject: [PATCH] [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 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/display/api.js b/src/display/api.js index fa41abab7349e9..f65b8215d2dfdb 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); }