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

[api-minor] Add an option, in PDFDocumentProxy.cleanup, to allow fonts to remain attached to the DOM #13172

Merged
merged 3 commits into from
Apr 5, 2021

Conversation

Snuffleupagus
Copy link
Collaborator

@Snuffleupagus Snuffleupagus commented Apr 2, 2021

As mentioned in the JSDoc comment, this should not be used unless you know what you're doing, since it will lead to increased memory usage. However, in some situations (e.g. SVG-rendering), we still want to be able to run general clean-up on both the main/worker-thread while keeping loaded fonts attached to the DOM.[1]

As part of these changes, WorkerTransport.startCleanup is converted to an async method and we'll also skip clean-up when destruction has started (since it's redundant).


[1] The SVG-rendering mode is obviously not officially supported, since it's both rather incomplete and inherently slower. However with recent changes, whereby we cache repeated images on the document rather than the page level, memory usage can be a lot worse than before if we never attempt to release e.g. cached image-data when the viewer is in SVG-rendering mode.

These two properties were *never* intended to be anything but "private", hence it really cannot hurt to actually indicate that they're *not* part of any official API.
…nts to remain attached to the DOM

As mentioned in the JSDoc comment, this should not be used unless you know what you're doing, since it will lead to increased memory usage. However, in some situations (e.g. SVG-rendering), we still want to be able to run general clean-up on both the main/worker-thread while keeping loaded fonts attached to the DOM.[1]

As part of these changes, `WorkerTransport.startCleanup` is converted to an async method and we'll also skip clean-up when destruction has started (since it's redundant).

---
[1] The SVG-rendering mode is obviously not officially supported, since it's both rather incomplete and inherently slower. However with recent changes, whereby we cache repeated images on the document rather than the page level, memory usage can be *a lot* worse than before if we never attempt to release e.g. cached image-data when the viewer is in SVG-rendering mode.
@Snuffleupagus
Copy link
Collaborator Author

/botio unittest

@pdfjsbot
Copy link

pdfjsbot commented Apr 2, 2021

From: Bot.io (Windows)


Received

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

Live output at: http://3.101.106.178:8877/162fba9695290ad/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Apr 2, 2021

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/fc420404f90c703/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Apr 2, 2021

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/fc420404f90c703/output.txt

Total script time: 3.71 mins

  • Unit Tests: Passed

@pdfjsbot
Copy link

pdfjsbot commented Apr 2, 2021

From: Bot.io (Windows)


Success

Full output at http://3.101.106.178:8877/162fba9695290ad/output.txt

Total script time: 6.77 mins

  • Unit Tests: Passed

@Snuffleupagus Snuffleupagus marked this pull request as ready for review April 3, 2021 11:32
@Snuffleupagus
Copy link
Collaborator Author

This is inspired by, and extends, the functionality implemented in https://github.com/mozilla/pdf.js/pull/13146/files#diff-082d6b37ad01db7ac97cc07c6ddb0dc52040484c5ef91b110b072f50144d9f39 since as outlined above this makes general sense to expose in the API.
Depending on how soon that PR will land, we should perhaps hold off on this one to avoid unnecessary merge conflicts!?

@timvandermeij timvandermeij merged commit 228adbf into mozilla:master Apr 5, 2021
@timvandermeij
Copy link
Contributor

timvandermeij commented Apr 5, 2021

Looks good! Given that a new release with [api-minor] change is coming up, I would like to have this included, and it will also make the XFA patch a bit smaller.

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