[api-minor] Remove the postMessageTransfers parameter, and thus the ability to manually disable transferring of data, from the API
#11123
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…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.
…eHandler.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.
|
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/eaf14e1511ad622/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/0441ae35294acd4/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/0441ae35294acd4/output.txt Total script time: 17.56 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/eaf14e1511ad622/output.txt Total script time: 26.01 mins
|
… 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
Snuffleupagus
force-pushed
the
rm-api-postMessageTransfers
branch
from
a598596
to
7dea3f9
Compare
timvandermeij
approved these changes
Sep 6, 2019
|
Looks good! I also don't see any reason to keep this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By transferring, rather than copying,
ArrayBuffers 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 callinggetDocument, is a performance footgun[1] which will do nothing but waste memory.Given that every reasonably modern browser supports
postMessagetransfers[2], I really don't see why it should be possible to force-disable this functionality.Looking at the browser support, for
postMessagetransfers[2], it's highly unlikely that PDF.js is even usable in browsers without it. However, the feature testing ofpostMessagetransfers is kept for the time being just to err on the safe side.[1] This is somewhat similar to the, now removed,
disableWorkerparameter 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