Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert {BaseViewer, PDFThumbnailViewer}._pagesRequests from an Array to a WeakMap #11350

Merged
merged 1 commit into from
Nov 21, 2019

Conversation

Snuffleupagus
Copy link
Collaborator

Obviously the _pagesRequests functionality is mainly used when disableAutoFetch is set, but it will also be used during ranged/streamed loading of documents.
However, the _pagesRequests property is currently an Array which seems a bit strange:

  • Arrays are zero-indexed, but the first element will never actually be set in the code.
  • The _pagesRequests Array is never cleared, unless a new document is loaded, and once the PDFDocumentProxy.getPage call has resolved/rejected the element is just replaced by null.
  • Unless the document is browsed in order the resulting _pagesRequests Array can also be arbitrarily sparse.

All in all, I don't believe that an Array is an appropriate data structure to use for this purpose.

…ay to a WeakMap

Obviously the `_pagesRequests` functionality is *mainly* used when `disableAutoFetch` is set, but it will also be used during ranged/streamed loading of documents.
However, the `_pagesRequests` property is currently an Array which seems a bit strange:

 - Arrays are zero-indexed, but the first element will never actually be set in the code.
 - The `_pagesRequests` Array is never cleared, unless a new document is loaded, and once the `PDFDocumentProxy.getPage` call has resolved/rejected the element is just replaced by `null`.
 - Unless the document is browsed *in order* the resulting `_pagesRequests` Array can also be arbitrarily sparse.

All in all, I don't believe that an Array is an appropriate data structure to use for this purpose.
@Snuffleupagus
Copy link
Collaborator Author

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/106ad4cc926949c/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/106ad4cc926949c/output.txt

Total script time: 1.70 mins

Published

@timvandermeij timvandermeij merged commit 96c5954 into mozilla:master Nov 21, 2019
@timvandermeij
Copy link
Contributor

Good improvement, thanks!

@Snuffleupagus Snuffleupagus deleted the _pagesRequests-WeakMap branch November 21, 2019 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants