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] Immediately release the font.data property once the font been attached to the DOM (PR 11777 follow-up) #11844

Merged
merged 1 commit into from
Apr 23, 2020

Conversation

Snuffleupagus
Copy link
Collaborator

This patch implements #11777 (comment)

This extends the work from PR #11773 and #11777 further, by immediately releasing the font.data property once the font been attached to the DOM. By not unnecessarily holding onto this data on the main-thread, we'll thus reduce the memory usage of fonts even further (especially beneficial in longer documents with composite fonts).

The new behaviour is controlled by the recently added fontExtraProperties API option (adding a new option just for this patch didn't seem necessary), since there's one edge-case in the SVG renderer where the font.data property is necessary (see the pdf2svg example).

Note that while the default viewer does run clean-up with an idle timeout, that timeout will be reset whenever rendering occurs or when scrolling happens in the viewer. In practice this means that unless the user doesn't interact with the viewer in any way during an extended period of time, currently set to 30 seconds, the PDFDocumentProxy.cleanup method will never be called and font resources will thus not be cleaned-up.

…t been attached to the DOM (PR 11777 follow-up)

*This patch implements mozilla#11777 (comment)

This extends the work from PR 11773 and 11777 further, by immediately releasing the `font.data` property once the font been attached to the DOM. By not unnecessarily holding onto this data on the main-thread, we'll thus reduce the memory usage of fonts even further (especially beneficial in longer documents with composite fonts).

The new behaviour is controlled by the recently added `fontExtraProperties` API option (adding a new option just for this patch didn't seem necessary), since there's one edge-case in the SVG renderer where the `font.data` property is necessary (see the `pdf2svg` example).

Note that while the default viewer does run clean-up with an idle timeout, that timeout will be reset whenever rendering occurs *or* when scrolling happens in the viewer. In practice this means that unless the user doesn't interact with the viewer in *any* way during an extended period of time, currently set to 30 seconds, the `PDFDocumentProxy.cleanup` method will never be called and font resources will thus not be cleaned-up.
@Snuffleupagus
Copy link
Collaborator Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/0e4636892428946/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

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

Total script time: 19.56 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/c915efa096af8e5/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/0e4636892428946/output.txt

Total script time: 24.98 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/0e4636892428946/reftest-analyzer.html#web=eq.log

@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/3095bf6d75fa79b/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/3095bf6d75fa79b/output.txt

Total script time: 2.56 mins

Published

@timvandermeij timvandermeij merged commit cd28787 into mozilla:master Apr 23, 2020
@timvandermeij
Copy link
Contributor

Looks like a good idea to me too. The sooner we can release unneeded resources, the better. Thanks!

@Snuffleupagus Snuffleupagus deleted the API-release-font-data branch April 24, 2020 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants