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.

Sign up for GitHub

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

A small memory-usage improvement for PDF documents opened from TypedArray-data #14968

Merged
merged 1 commit into from
May 29, 2022
Merged

A small memory-usage improvement for PDF documents opened from TypedArray-data #14968

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/display/api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,12 @@ async function _fetchDocument(worker, source, pdfDataRangeTransport, docId) {
}
);

// Release the TypedArray data, when it exists, since it's no longer needed
// on the main-thread *after* it's been sent to the worker-thread.
if (source.data) {
source.data = null;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it make sense to transfer the data in the worker ?

Copy link
Collaborator Author

@Snuffleupagus Snuffleupagus May 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I though about that, but decided against doing it since that may end up breaking someone's code. Given that can't know how third-party users are invoking getDocument, it's thus possible that a user want to do something with the TypedArray after calling getDocument and it'd be surprising (and a breaking API-change) if we just transferred this data.

(And note that none of this applies to the Firefox PDF viewer either.)

if (worker.destroyed) {
throw new Error("Worker was destroyed");
}
Expand Down Expand Up @@ -953,8 +959,8 @@ class PDFDocumentProxy {
}

/**
* @returns {Promise<TypedArray>} A promise that is resolved with a
* {TypedArray} that has the raw data from the PDF.
* @returns {Promise<Uint8Array>} A promise that is resolved with a
* {Uint8Array} that has the raw data from the PDF.
Comment on lines +962 to +963
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While unrelated to the rest of the patch, this method should always be returning a Uint8Array hence it cannot hurt to be more specific in the JSDocs.

*/
getData() {
return this._transport.getData();
Expand Down