Skip to content

Commit

Permalink
[api-minor] Remove the postMessageTransfers parameter, and thus the…
Browse files Browse the repository at this point in the history
… 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
  • Loading branch information
Snuffleupagus committed Sep 5, 2019
1 parent f0534b9 commit 7dea3f9
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 13 deletions.
11 changes: 3 additions & 8 deletions src/display/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -319,7 +317,6 @@ function getDocument(src) {

if (!worker) {
const workerParams = {
postMessageTransfers: params.postMessageTransfers,
verbosity: params.verbosity,
port: GlobalWorkerOptions.workerPort,
};
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -1568,15 +1563,15 @@ 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');
}

this.name = name;
this.destroyed = false;
this.postMessageTransfers = postMessageTransfers !== false;
this.postMessageTransfers = true;
this.verbosity = verbosity;

this._readyCapability = createPromiseCapability();
Expand Down Expand Up @@ -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);
}
Expand Down
5 changes: 0 additions & 5 deletions web/app_options.js
Original file line number Diff line number Diff line change
Expand Up @@ -206,11 +206,6 @@ const defaultOptions = {
value: false,
kind: OptionKind.API,
},
postMessageTransfers: {
/** @type {boolean} */
value: true,
kind: OptionKind.API,
},
verbosity: {
/** @type {number} */
value: 1,
Expand Down

0 comments on commit 7dea3f9

Please sign in to comment.