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

Cache JPEG images, just as we do for other image formats, in evaluator.js (issue 8380) #8388

Merged
merged 1 commit into from
May 17, 2017
Merged

Conversation

Snuffleupagus
Copy link
Collaborator

For some reason, we're putting all kind of images except JPEG into the imageCache in evaluator.js.[1]
This means that in the PDF file in issue #8380, we'll keep sending the same two small images[2] to the main-thread and decoding them over and over. This is obviously hugely inefficient!

As can be seen from the discussion in the issue, the performance becomes extremely bad if the user has the addon "Adblock Plus" installed. However, even in a clean Firefox profile, the performance isn't that great.

This patch not only addresses the performance implications of the "Adblock Plus" addon together with that particular PDF file, but it also improves the rendering times considerably for all users.
Locally, with a clean profile, the rendering times are reduced from ~2000 ms to ~500 ms for my setup!

Obviously, the general structure of the PDF file and its operator sequence is still hugely inefficient, however I'd say that the performance with this patch is good enough to consider the issue (as it stands) resolved.[3]

Fixes #8380.


[1] Not technically true, since inline images are cached from parser.js, but whatever :-)

[2] The two JPEG images have dimensions 1x2, respectively 4x2.

[3] To make this even more efficient, a new state would have to be added to the QueueOptimizer. Given that PDF files this stupid fortunately aren't too common, I'm not convinced that it's worth doing.

…or.js` (issue 8380)

For some reason, we're putting all kind of images *except* JPEG into the `imageCache` in `evaluator.js`.[1]
This means that in the PDF file in issue 8380, we'll keep sending the *same* two small images[2] to the main-thread and decoding them over and over. This is obviously hugely inefficient!

As can be seen from the discussion in the issue, the performance becomes *extremely* bad if the user has the addon "Adblock Plus" installed. However, even in a clean Firefox profile, the performance isn't that great.

This patch not only addresses the performance implications of the "Adblock Plus" addon together with that particular PDF file, but it *also* improves the rendering times considerably for *all* users.
Locally, with a clean profile, the rendering times are reduced from `~2000 ms` to `~500 ms` for my setup!

Obviously, the general structure of the PDF file and its operator sequence is still hugely inefficient, however I'd say that the performance with this patch is good enough to consider the issue (as it stands) resolved.[3]

Fixes 8380.

---
[1] Not technically true, since inline images are cached from `parser.js`, but whatever :-)

[2] The two JPEG images have dimensions 1x2, respectively 4x2.

[3] To make this even more efficient, a new state would have to be added to the `QueueOptimizer`. Given that PDF files this stupid fortunately aren't too common, I'm not convinced that it's worth doing.
@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/db82c90fbe08a1f/output.txt

@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/4b3710c80d047a4/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

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

Total script time: 16.68 mins

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://54.215.176.217:8877/4b3710c80d047a4/output.txt

Total script time: 25.81 mins

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

@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented May 12, 2017

One thing I forgot to mention in #8388 (comment), is that if you disable the NativeImageDecoder then all image formats, including JPEG, are already cached as OPS.paintImageXObject with the current master.

What this patch does is simply to ensure that we also cache JPEG images, as OPS.paintJpegXObject, when the NativeImageDecoder is enabled (which is the default).

@yurydelendik
Copy link
Contributor

/botio makeref

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_makeref from @yurydelendik received. Current queue size: 0

Live output at: http://54.67.70.0:8877/9e82d0040872c3f/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_makeref from @yurydelendik received. Current queue size: 1

Live output at: http://54.215.176.217:8877/4d4869fa3fd3560/output.txt

@yurydelendik yurydelendik merged commit 5dc8dcd into mozilla:master May 17, 2017
@yurydelendik
Copy link
Contributor

Thank you for the patch.

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://54.215.176.217:8877/4d4869fa3fd3560/output.txt

Total script time: 24.51 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@Snuffleupagus Snuffleupagus deleted the issue-8380 branch May 17, 2017 23:18
@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/9e82d0040872c3f/output.txt

Total script time: 60.00 mins

  • Lint: Passed

@brendandahl
Copy link
Contributor

/botio-linux makeref

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_makeref from @brendandahl received. Current queue size: 0

Live output at: http://54.67.70.0:8877/2d422734914b925/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/2d422734914b925/output.txt

Total script time: 0.09 mins

@brendandahl
Copy link
Contributor

/botio-linux makeref

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_makeref from @brendandahl received. Current queue size: 0

Live output at: http://54.67.70.0:8877/21c62528934c340/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/21c62528934c340/output.txt

Total script time: 0.08 mins

@brendandahl
Copy link
Contributor

/botio-linux makeref

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_makeref from @brendandahl received. Current queue size: 0

Live output at: http://54.67.70.0:8877/44014aed63cb13a/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/44014aed63cb13a/output.txt

Total script time: 0.09 mins

@Snuffleupagus Snuffleupagus restored the issue-8380 branch May 18, 2017 18:54
@Snuffleupagus
Copy link
Collaborator Author

Sorry; I've just restored the branch, let's try again.

/botio-linux makeref

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

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

Total script time: 23.50 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@Snuffleupagus Snuffleupagus deleted the issue-8380 branch May 19, 2017 06:01
movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
Cache JPEG images, just as we do for other image formats, in `evaluator.js` (issue 8380)
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.

Horrible performance with a pdf document
4 participants