Attempt to further reduce re-parsing for globally cached images (PR 11912, 16108 follow-up) #17428

Snuffleupagus · 2023-12-15T09:29:01Z

In PR #11912 we started caching images that occur on multiple pages globally, which improved performance a lot in many PDF documents.
However, one slightly annoying limitation of the implementation is the need to re-parse the image once the global-caching threshold has been reached. Previously this was difficult to avoid, since large image-resources will cause cleanup to run on the main-thread after rendering has finished. In PR #16108 we started delaying this cleanup a little bit, to improve performance if a user e.g. zooms and/or rotates the document immediately after rendering completes.

Taking those two PRs together, we now have a situation where it's much more likely that the main-thread has "globally used" images cached at the page-level. Hence we can instead attempt to copy a locally cached image into the global object-cache on the main-thread and thus reduce unnecessary re-parsing of large/complex global images, which significantly reduces the rendering time in many cases.

For the PDF document in issue #11878, the rendering time of the second page changes as follows (on my computer):

With the master-branch it takes >600 ms to render.
With this patch that goes down to ~50 ms, which is one order of magnitude faster.

(Note that all other pages are, as expected, completely unaffected by these changes.)

This new main-thread copying is limited to "large" global images, since:

Re-parsing of small images, on the worker-thread, is usually fast enough to not be an issue.
With the delayed cleanup after rendering, it's still not guaranteed that an image is available in a page-level cache on the main-thread.
This forces the worker-thread to wait for the main-thread, which is a pattern that you always want to avoid unless absolutely necessary.

moz-tools-bot · 2023-12-15T10:29:28Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/5b4fe913e37c482/output.txt

moz-tools-bot · 2023-12-15T10:29:28Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/6c7e30a1cdbed19/output.txt

moz-tools-bot · 2023-12-15T10:53:23Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/5b4fe913e37c482/output.txt

Total script time: 23.91 mins

Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

  different ref/snapshot: 12
  different first/second rendering: 1

Image differences available at: http://54.241.84.105:8877/5b4fe913e37c482/reftest-analyzer.html#web=eq.log

moz-tools-bot · 2023-12-15T11:07:06Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/6c7e30a1cdbed19/output.txt

Total script time: 37.63 mins

Unit tests: Passed
Integration Tests: FAILED
Regression tests: FAILED

  different ref/snapshot: 24

Image differences available at: http://54.193.163.58:8877/6c7e30a1cdbed19/reftest-analyzer.html#web=eq.log

src/display/api.js

calixteman · 2023-12-21T20:15:28Z

The pdf in #11878 is probably a bit unusual, I mean I'm not sure (it's just an opinion) that there are so many pdfs in the wild having a common big image on several pages.
Do you know if there are some improvements in other context (e.g. when rotating ? zooming ?...) ? or with more usual pdfs ?
Do you know if there are some possibles regressions (possibly due to the added sync point in the worker) ?

This (obviously) only includes "resolved" data, and will be used in an upcoming patch.

Currently this is done in the API, but moving it into the worker-thread will simplify upcoming changes.

…1912, 16108 follow-up) In PR 11912 we started caching images that occur on multiple pages globally, which improved performance a lot in many PDF documents. However, one slightly annoying limitation of the implementation is the need to re-parse the image once the global-caching threshold has been reached. Previously this was difficult to avoid, since large image-resources will cause cleanup to run on the main-thread after rendering has finished. In PR 16108 we started delaying this cleanup a little bit, to improve performance if a user e.g. zooms and/or rotates the document immediately after rendering completes. Taking those two PRs together, we now have a situation where it's much more likely that the main-thread has "globally used" images cached at the page-level. Hence we can instead attempt to *copy* a locally cached image into the global object-cache on the main-thread and thus reduce unnecessary re-parsing of large/complex global images, which significantly reduces the rendering time in many cases. For the PDF document in issue 11878, the rendering time of *the second page* changes as follows (on my computer): - With the `master`-branch it takes >600 ms to render. - With this patch that goes down to ~50 ms, which is one order of magnitude faster. (Note that all other pages are, as expected, completely unaffected by these changes.) This new main-thread copying is limited to "large" global images, since: - Re-parsing of small images, on the worker-thread, is usually fast enough to not be an issue. - With the delayed cleanup after rendering, it's still not guaranteed that an image is available in a page-level cache on the main-thread. - This forces the worker-thread to wait for the main-thread, which is a pattern that you always want to avoid unless absolutely necessary.

Snuffleupagus · 2023-12-21T20:40:32Z

The pdf in #11878 is probably a bit unusual, I mean I'm not sure (it's just an opinion) that there are so many pdfs in the wild having a common big image on several pages.

While it's difficult for me to say how common this is, we've seen a number of cases (in both GitHub and Bugzilla) where the same large image is used as the background on all pages; note the issues/bugs listed at the end of #11912 (comment) and those are the ones I knew about three years ago.

Hence I'd say that a global image-cache definitely helps, to both reduce CPU and memory usage, and this patch simply improves things a little bit further.

Do you know if there are some improvements in other context (e.g. when rotating ? zooming ?...) ? or with more usual pdfs ?

This patch won't really affect those cases, as far as I can tell.

Do you know if there are some possibles regressions (possibly due to the added sync point in the worker) ?

There shouldn't be, since only images that we've previously seen at least once on another page will trigger this new code-path.

calixteman

LGTM. Thank you.

calixteman · 2023-12-21T20:58:25Z

Just a side note, wdyt about using Map objects for #objs and similar ?

Snuffleupagus · 2023-12-21T21:22:35Z

Just a side note, wdyt about using Map objects for #objs and similar ?

I've also been thinking about that, but as far as I know Map (and Set) is still slower than an Object; see https://bugzilla.mozilla.org/show_bug.cgi?id=1799154

Snuffleupagus · 2023-12-21T21:22:47Z

/botio test

moz-tools-bot · 2023-12-21T21:22:50Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/1dcd8cb0b32524f/output.txt

moz-tools-bot · 2023-12-21T21:22:50Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/ea8609c246f872f/output.txt

moz-tools-bot · 2023-12-21T21:25:29Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/1dcd8cb0b32524f/output.txt

Total script time: 2.64 mins

Unit tests: FAILED
Integration Tests: FAILED
Regression tests: FAILED

Image differences available at: http://54.193.163.58:8877/1dcd8cb0b32524f/reftest-analyzer.html#web=eq.log

moz-tools-bot · 2023-12-21T21:46:45Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/ea8609c246f872f/output.txt

Total script time: 23.90 mins

Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

  different ref/snapshot: 14
  different first/second rendering: 2

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

calixteman · 2023-12-21T22:51:36Z

/botio-windows test

moz-tools-bot · 2023-12-21T22:51:39Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/8685011bf36e9e3/output.txt

moz-tools-bot · 2023-12-21T23:29:18Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/8685011bf36e9e3/output.txt

Total script time: 37.64 mins

Unit tests: Passed
Integration Tests: FAILED
Regression tests: FAILED

  different ref/snapshot: 27

Image differences available at: http://54.193.163.58:8877/8685011bf36e9e3/reftest-analyzer.html#web=eq.log

Snuffleupagus added performance image-conversion labels Dec 15, 2023

Snuffleupagus force-pushed the cacheGlobally-CopyImage branch 2 times, most recently from d0bf33c to e2d7e15 Compare December 15, 2023 09:55

mozilla deleted a comment from moz-tools-bot Dec 15, 2023

Snuffleupagus requested a review from calixteman December 21, 2023 17:51

calixteman reviewed Dec 21, 2023

View reviewed changes

src/display/api.js Outdated Show resolved Hide resolved

Snuffleupagus added 3 commits December 21, 2023 21:26

Add iteration support in the PDFObjects class

b09f238

This (obviously) only includes "resolved" data, and will be used in an upcoming patch.

Compute the length of the final image-bitmap/data on the worker-thread

e547b19

Currently this is done in the API, but moving it into the worker-thread will simplify upcoming changes.

Snuffleupagus force-pushed the cacheGlobally-CopyImage branch from e2d7e15 to 9f02cc3 Compare December 21, 2023 20:26

calixteman approved these changes Dec 21, 2023

View reviewed changes

Snuffleupagus merged commit faa24e8 into mozilla:master Dec 22, 2023
9 checks passed

Snuffleupagus deleted the cacheGlobally-CopyImage branch December 22, 2023 09:25

Snuffleupagus mentioned this pull request Apr 20, 2024

Extend the globally cached image main-thread copying to "complex" images as well (PR 17428 follow-up) #17978

Merged

plantago mentioned this pull request May 2, 2024

Rendering crashes if PDF contains images which cannot be decoded #18042

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attempt to further reduce re-parsing for globally cached images (PR 11912, 16108 follow-up) #17428

Attempt to further reduce re-parsing for globally cached images (PR 11912, 16108 follow-up) #17428

Snuffleupagus commented Dec 15, 2023

moz-tools-bot commented Dec 15, 2023

moz-tools-bot commented Dec 15, 2023

moz-tools-bot commented Dec 15, 2023

moz-tools-bot commented Dec 15, 2023

calixteman commented Dec 21, 2023

Snuffleupagus commented Dec 21, 2023

calixteman left a comment

calixteman commented Dec 21, 2023

Snuffleupagus commented Dec 21, 2023

Snuffleupagus commented Dec 21, 2023

moz-tools-bot commented Dec 21, 2023

moz-tools-bot commented Dec 21, 2023

moz-tools-bot commented Dec 21, 2023

moz-tools-bot commented Dec 21, 2023

calixteman commented Dec 21, 2023

moz-tools-bot commented Dec 21, 2023

moz-tools-bot commented Dec 21, 2023

Attempt to further reduce re-parsing for globally cached images (PR 11912, 16108 follow-up) #17428

Attempt to further reduce re-parsing for globally cached images (PR 11912, 16108 follow-up) #17428

Conversation

Snuffleupagus commented Dec 15, 2023

moz-tools-bot commented Dec 15, 2023

From: Bot.io (Linux m4)

Received

moz-tools-bot commented Dec 15, 2023

From: Bot.io (Windows)

Received

moz-tools-bot commented Dec 15, 2023

From: Bot.io (Linux m4)

Failed

moz-tools-bot commented Dec 15, 2023

From: Bot.io (Windows)

Failed

calixteman commented Dec 21, 2023

Snuffleupagus commented Dec 21, 2023

calixteman left a comment

Choose a reason for hiding this comment

calixteman commented Dec 21, 2023

Snuffleupagus commented Dec 21, 2023

Snuffleupagus commented Dec 21, 2023

moz-tools-bot commented Dec 21, 2023

From: Bot.io (Windows)

Received

moz-tools-bot commented Dec 21, 2023

From: Bot.io (Linux m4)

Received

moz-tools-bot commented Dec 21, 2023

From: Bot.io (Windows)

Failed

moz-tools-bot commented Dec 21, 2023

From: Bot.io (Linux m4)

Failed

calixteman commented Dec 21, 2023

moz-tools-bot commented Dec 21, 2023

From: Bot.io (Windows)

Received

moz-tools-bot commented Dec 21, 2023

From: Bot.io (Windows)

Failed