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

Improve (local) caching of parsed ColorSpaces (PR 12001 follow-up) #12012

Merged
merged 4 commits into from
Jun 24, 2020

Conversation

Snuffleupagus
Copy link
Collaborator

@Snuffleupagus Snuffleupagus commented Jun 18, 2020

This patch contains the following notable improvements:

  • Changes the ColorSpace.parse call-sites to, where possible, pass in a reference rather than actual ColorSpace data (necessary for the next point).
  • Adds (local) caching of ColorSpaces by Ref, when applicable, in addition the caching by name. This (generally) improves ColorSpace caching for e.g. the SMask code-paths.
  • Extends the (local) ColorSpace caching to also apply when handling Images and Patterns, thus further reducing unneeded re-parsing.
  • Adds a new ColorSpace.parseAsync method, almost identical to the existing ColorSpace.parse one, but returning a Promise instead (this simplifies some code in the PartialEvaluator).

@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/aecf22498af18e1/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/3ffee2b80de75bb/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/aecf22498af18e1/output.txt

Total script time: 28.06 mins

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

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

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/3ffee2b80de75bb/output.txt

Total script time: 60.00 mins

@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/68f0aaf45eb8443/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/f4a9cf9a23f9609/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

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

Total script time: 25.44 mins

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/68f0aaf45eb8443/output.txt

Total script time: 29.72 mins

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

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

@Snuffleupagus Snuffleupagus marked this pull request as ready for review June 19, 2020 09:55
@Snuffleupagus Snuffleupagus force-pushed the ColorSpace-parse-cache branch 3 times, most recently from eb3a6ff to 39a1587 Compare June 22, 2020 09:32
@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/95a68e86860315d/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/4802e4c61eb3764/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/95a68e86860315d/output.txt

Total script time: 25.98 mins

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/4802e4c61eb3764/output.txt

Total script time: 29.25 mins

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

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

@Snuffleupagus Snuffleupagus force-pushed the ColorSpace-parse-cache branch 2 times, most recently from bf74dc6 to 8adab78 Compare June 23, 2020 20:13
@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/a86eced09e86690/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/8928e8a71692b26/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

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

Total script time: 25.73 mins

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/8928e8a71692b26/output.txt

Total script time: 29.24 mins

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

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

…a bunch of (randomly) ordered parameters

Given the number of existing parameters, this will avoid needlessly unwieldy call-sites especially with upcoming changes in later patches.
…n the image one (PR 12001 follow-up)

This will allow caching of ColorSpaces by either `Name` *or* `Ref`, which doesn't really make sense for images, thus allowing (better) caching for ColorSpaces used with e.g. Images and Patterns.
This patch contains the following *notable* improvements:
 - Changes the `ColorSpace.parse` call-sites to, where possible, pass in a reference rather than actual ColorSpace data (necessary for the next point).
 - Adds (local) caching of `ColorSpace`s by `Ref`, when applicable, in addition the caching by name. This (generally) improves `ColorSpace` caching for e.g. the SMask code-paths.
 - Extends the (local) `ColorSpace` caching to also apply when handling Images and Patterns, thus further reducing unneeded re-parsing.
 - Adds a new `ColorSpace.parseAsync` method, almost identical to the existing `ColorSpace.parse` one, but returning a Promise instead (this simplifies some code in the `PartialEvaluator`).
…ntry of `ICCBased` ColorSpaces (PR 9659 follow-up)

With the changes made in PR 9659, `ColorSpace.fromIR` no longer takes a second `pdfFunctionFactory` parameter and there's thus one call-site that can be simplified.
@timvandermeij timvandermeij merged commit 276d917 into mozilla:master Jun 24, 2020
@timvandermeij
Copy link
Contributor

Thank you for working on this!

/botio makeref

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/b8958604b42aa14/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

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

Total script time: 24.06 mins

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/b8958604b42aa14/output.txt

Total script time: 27.64 mins

  • Lint: Passed
  • Make references: FAILED

@Snuffleupagus
Copy link
Collaborator Author

/botio-windows makeref

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/9b87ddc8169ad8d/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://54.215.176.217:8877/9b87ddc8169ad8d/output.txt

Total script time: 26.91 mins

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

@Snuffleupagus Snuffleupagus deleted the ColorSpace-parse-cache branch June 24, 2020 23:32
@Snuffleupagus
Copy link
Collaborator Author

Thank you for working on this!

As always, thanks for landing the PR!

(I'm currently looking into caching of parsed Functions, which may further improve some documents.)

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