Add local caching of ColorSpaces, by name, in PartialEvaluator.getOperatorList (issue 2504)
#12001
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…OperatorList` (issue 2504) By caching parsed `ColorSpace`s, we thus don't need to re-parse the same data over and over which saves CPU cycles *and* reduces peak memory usage. (Obviously persistent memory usage *may* increase a tiny bit, but since the caching is done per `PartialEvaluator.getOperatorList` invocation and given that `ColorSpace` instances generally hold very little data this shouldn't be much of an issue.) Furthermore, by caching `ColorSpace`s we can also lookup the already parsed ones *synchronously* during the `OperatorList` building, instead of having to defer to the event loop/microtask queue since the parsing is done asynchronously (such that error handling is easier). Possible future improvements: - Cache/lookup parsed `ColorSpaces` used in `Pattern`s and `Image`s. - Attempt to cache *local* `ColorSpace`s by reference as well, in addition to only by name, assuming that there's documents where that would be beneficial and that it's not too difficult to implement. - Assuming there's documents that would benefit from it, also cache repeated `ColorSpace`s *globally* as well. Given that we've never, until now, been doing *any* caching of parsed `ColorSpace`s and that even using a simple name-only *local* cache helps tremendously in pathological cases, I purposely decided against complicating the implementation too much initially. Also, compared to parsing of `Image`s, simply creating a `ColorSpace` instance isn't that expensive (hence I'd be somewhat surprised if adding a *global* cache would help much). --- This patch was tested using: - The default `tracemonkey` PDF file, which was included mostly to show that "normal" documents aren't negatively affected by these changes. - The PDF file from issue 2504, i.e. https://dl-ctlg.panasonic.com/jp/manual/sd/sd_rbm1000_0.pdf, where most pages will switch *thousands* of times between a handful of `ColorSpace`s. with the following manifest file: ``` [ { "id": "tracemonkey", "file": "pdfs/tracemonkey.pdf", "md5": "9a192d8b1a7dc652a19835f6f08098bd", "rounds": 100, "type": "eq" }, { "id": "issue2504", "file": "../web/pdfs/issue2504.pdf", "md5": "", "rounds": 20, "type": "eq" } ] ``` which gave the following results when comparing this patch against the `master` branch: - Overall ``` -- Grouped By browser, pdf, stat -- browser | pdf | stat | Count | Baseline(ms) | Current(ms) | +/- | % | Result(P<.05) ------- | ----------- | ------------ | ----- | ------------ | ----------- | ---- | ------ | ------------- firefox | issue2504 | Overall | 640 | 977 | 497 | -479 | -49.08 | faster firefox | issue2504 | Page Request | 640 | 3 | 4 | 1 | 59.18 | firefox | issue2504 | Rendering | 640 | 974 | 493 | -481 | -49.37 | faster firefox | tracemonkey | Overall | 1400 | 116 | 111 | -5 | -4.43 | firefox | tracemonkey | Page Request | 1400 | 2 | 2 | 0 | -2.86 | firefox | tracemonkey | Rendering | 1400 | 114 | 109 | -5 | -4.47 | ``` - Page-specific ``` -- Grouped By browser, pdf, page, stat -- browser | pdf | page | stat | Count | Baseline(ms) | Current(ms) | +/- | % | Result(P<.05) ------- | ----------- | ---- | ------------ | ----- | ------------ | ----------- | ----- | ------- | ------------- firefox | issue2504 | 0 | Overall | 20 | 2295 | 1268 | -1027 | -44.76 | faster firefox | issue2504 | 0 | Page Request | 20 | 6 | 7 | 1 | 15.32 | firefox | issue2504 | 0 | Rendering | 20 | 2288 | 1260 | -1028 | -44.93 | faster firefox | issue2504 | 1 | Overall | 20 | 3059 | 2806 | -252 | -8.25 | faster firefox | issue2504 | 1 | Page Request | 20 | 11 | 14 | 3 | 23.25 | slower firefox | issue2504 | 1 | Rendering | 20 | 3047 | 2792 | -255 | -8.37 | faster firefox | issue2504 | 2 | Overall | 20 | 411 | 295 | -116 | -28.20 | faster firefox | issue2504 | 2 | Page Request | 20 | 2 | 42 | 40 | 1897.62 | firefox | issue2504 | 2 | Rendering | 20 | 409 | 253 | -156 | -38.09 | faster firefox | issue2504 | 3 | Overall | 20 | 736 | 299 | -437 | -59.34 | faster firefox | issue2504 | 3 | Page Request | 20 | 2 | 2 | 0 | 0.00 | firefox | issue2504 | 3 | Rendering | 20 | 734 | 297 | -437 | -59.49 | faster firefox | issue2504 | 4 | Overall | 20 | 356 | 458 | 102 | 28.63 | firefox | issue2504 | 4 | Page Request | 20 | 1 | 2 | 1 | 57.14 | slower firefox | issue2504 | 4 | Rendering | 20 | 354 | 455 | 101 | 28.53 | firefox | issue2504 | 5 | Overall | 20 | 1381 | 765 | -616 | -44.59 | faster firefox | issue2504 | 5 | Page Request | 20 | 3 | 5 | 2 | 50.00 | slower firefox | issue2504 | 5 | Rendering | 20 | 1378 | 760 | -617 | -44.81 | faster firefox | issue2504 | 6 | Overall | 20 | 757 | 299 | -459 | -60.57 | faster firefox | issue2504 | 6 | Page Request | 20 | 2 | 5 | 3 | 150.00 | slower firefox | issue2504 | 6 | Rendering | 20 | 755 | 294 | -462 | -61.11 | faster firefox | issue2504 | 7 | Overall | 20 | 394 | 302 | -92 | -23.39 | faster firefox | issue2504 | 7 | Page Request | 20 | 2 | 1 | -1 | -34.88 | faster firefox | issue2504 | 7 | Rendering | 20 | 392 | 301 | -91 | -23.32 | faster firefox | issue2504 | 8 | Overall | 20 | 2875 | 979 | -1896 | -65.95 | faster firefox | issue2504 | 8 | Page Request | 20 | 1 | 2 | 0 | 11.11 | firefox | issue2504 | 8 | Rendering | 20 | 2874 | 978 | -1896 | -65.99 | faster firefox | issue2504 | 9 | Overall | 20 | 700 | 332 | -368 | -52.60 | faster firefox | issue2504 | 9 | Page Request | 20 | 3 | 2 | 0 | -4.00 | firefox | issue2504 | 9 | Rendering | 20 | 698 | 329 | -368 | -52.78 | faster firefox | issue2504 | 10 | Overall | 20 | 3296 | 926 | -2370 | -71.91 | faster firefox | issue2504 | 10 | Page Request | 20 | 2 | 2 | 0 | -18.75 | firefox | issue2504 | 10 | Rendering | 20 | 3293 | 924 | -2370 | -71.96 | faster firefox | issue2504 | 11 | Overall | 20 | 524 | 197 | -327 | -62.34 | faster firefox | issue2504 | 11 | Page Request | 20 | 2 | 3 | 1 | 58.54 | firefox | issue2504 | 11 | Rendering | 20 | 522 | 194 | -328 | -62.81 | faster firefox | issue2504 | 12 | Overall | 20 | 752 | 369 | -384 | -50.98 | faster firefox | issue2504 | 12 | Page Request | 20 | 3 | 2 | -1 | -36.51 | faster firefox | issue2504 | 12 | Rendering | 20 | 749 | 367 | -382 | -51.05 | faster firefox | issue2504 | 13 | Overall | 20 | 679 | 487 | -193 | -28.38 | faster firefox | issue2504 | 13 | Page Request | 20 | 4 | 2 | -2 | -48.68 | faster firefox | issue2504 | 13 | Rendering | 20 | 676 | 485 | -191 | -28.28 | faster firefox | issue2504 | 14 | Overall | 20 | 474 | 283 | -191 | -40.26 | faster firefox | issue2504 | 14 | Page Request | 20 | 2 | 4 | 2 | 78.57 | firefox | issue2504 | 14 | Rendering | 20 | 471 | 279 | -192 | -40.79 | faster firefox | issue2504 | 15 | Overall | 20 | 860 | 618 | -241 | -28.05 | faster firefox | issue2504 | 15 | Page Request | 20 | 2 | 3 | 0 | 10.87 | firefox | issue2504 | 15 | Rendering | 20 | 857 | 616 | -241 | -28.15 | faster firefox | issue2504 | 16 | Overall | 20 | 389 | 243 | -147 | -37.71 | faster firefox | issue2504 | 16 | Page Request | 20 | 2 | 2 | 0 | 2.33 | firefox | issue2504 | 16 | Rendering | 20 | 387 | 240 | -147 | -37.94 | faster firefox | issue2504 | 17 | Overall | 20 | 1484 | 672 | -812 | -54.70 | faster firefox | issue2504 | 17 | Page Request | 20 | 2 | 3 | 1 | 37.21 | firefox | issue2504 | 17 | Rendering | 20 | 1482 | 669 | -812 | -54.84 | faster firefox | issue2504 | 18 | Overall | 20 | 575 | 252 | -323 | -56.12 | faster firefox | issue2504 | 18 | Page Request | 20 | 2 | 2 | 0 | -16.22 | firefox | issue2504 | 18 | Rendering | 20 | 573 | 251 | -322 | -56.24 | faster firefox | issue2504 | 19 | Overall | 20 | 517 | 227 | -290 | -56.08 | faster firefox | issue2504 | 19 | Page Request | 20 | 2 | 2 | 0 | 21.62 | firefox | issue2504 | 19 | Rendering | 20 | 515 | 225 | -290 | -56.37 | faster firefox | issue2504 | 20 | Overall | 20 | 668 | 670 | 2 | 0.31 | firefox | issue2504 | 20 | Page Request | 20 | 4 | 2 | -1 | -34.29 | firefox | issue2504 | 20 | Rendering | 20 | 664 | 667 | 3 | 0.49 | firefox | issue2504 | 21 | Overall | 20 | 486 | 309 | -177 | -36.44 | faster firefox | issue2504 | 21 | Page Request | 20 | 2 | 2 | 0 | 16.13 | firefox | issue2504 | 21 | Rendering | 20 | 484 | 307 | -177 | -36.60 | faster firefox | issue2504 | 22 | Overall | 20 | 543 | 267 | -276 | -50.85 | faster firefox | issue2504 | 22 | Page Request | 20 | 2 | 2 | 0 | 10.26 | firefox | issue2504 | 22 | Rendering | 20 | 541 | 265 | -276 | -51.07 | faster firefox | issue2504 | 23 | Overall | 20 | 3246 | 871 | -2375 | -73.17 | faster firefox | issue2504 | 23 | Page Request | 20 | 2 | 3 | 1 | 37.21 | firefox | issue2504 | 23 | Rendering | 20 | 3243 | 868 | -2376 | -73.25 | faster firefox | issue2504 | 24 | Overall | 20 | 379 | 156 | -223 | -58.83 | faster firefox | issue2504 | 24 | Page Request | 20 | 2 | 2 | 0 | -2.86 | firefox | issue2504 | 24 | Rendering | 20 | 378 | 154 | -223 | -59.10 | faster firefox | issue2504 | 25 | Overall | 20 | 176 | 127 | -50 | -28.19 | faster firefox | issue2504 | 25 | Page Request | 20 | 2 | 1 | 0 | -15.63 | firefox | issue2504 | 25 | Rendering | 20 | 175 | 125 | -49 | -28.31 | faster firefox | issue2504 | 26 | Overall | 20 | 181 | 108 | -74 | -40.67 | faster firefox | issue2504 | 26 | Page Request | 20 | 3 | 2 | -1 | -39.13 | faster firefox | issue2504 | 26 | Rendering | 20 | 178 | 105 | -72 | -40.69 | faster firefox | issue2504 | 27 | Overall | 20 | 208 | 104 | -104 | -49.92 | faster firefox | issue2504 | 27 | Page Request | 20 | 2 | 2 | 1 | 48.39 | firefox | issue2504 | 27 | Rendering | 20 | 206 | 102 | -104 | -50.64 | faster firefox | issue2504 | 28 | Overall | 20 | 241 | 111 | -131 | -54.16 | faster firefox | issue2504 | 28 | Page Request | 20 | 2 | 2 | -1 | -33.33 | firefox | issue2504 | 28 | Rendering | 20 | 239 | 109 | -130 | -54.39 | faster firefox | issue2504 | 29 | Overall | 20 | 321 | 196 | -125 | -39.05 | faster firefox | issue2504 | 29 | Page Request | 20 | 1 | 2 | 0 | 17.86 | firefox | issue2504 | 29 | Rendering | 20 | 319 | 194 | -126 | -39.35 | faster firefox | issue2504 | 30 | Overall | 20 | 651 | 271 | -380 | -58.41 | faster firefox | issue2504 | 30 | Page Request | 20 | 1 | 2 | 1 | 50.00 | firefox | issue2504 | 30 | Rendering | 20 | 649 | 269 | -381 | -58.60 | faster firefox | issue2504 | 31 | Overall | 20 | 1635 | 647 | -988 | -60.42 | faster firefox | issue2504 | 31 | Page Request | 20 | 1 | 2 | 0 | 30.43 | firefox | issue2504 | 31 | Rendering | 20 | 1634 | 645 | -988 | -60.49 | faster firefox | tracemonkey | 0 | Overall | 100 | 51 | 51 | 0 | 0.02 | firefox | tracemonkey | 0 | Page Request | 100 | 1 | 1 | 0 | -4.76 | firefox | tracemonkey | 0 | Rendering | 100 | 50 | 50 | 0 | 0.12 | firefox | tracemonkey | 1 | Overall | 100 | 97 | 91 | -5 | -5.52 | faster firefox | tracemonkey | 1 | Page Request | 100 | 3 | 3 | 0 | -1.32 | firefox | tracemonkey | 1 | Rendering | 100 | 94 | 88 | -5 | -5.73 | faster firefox | tracemonkey | 2 | Overall | 100 | 40 | 40 | 0 | 0.50 | firefox | tracemonkey | 2 | Page Request | 100 | 1 | 1 | 0 | 3.16 | firefox | tracemonkey | 2 | Rendering | 100 | 39 | 39 | 0 | 0.54 | firefox | tracemonkey | 3 | Overall | 100 | 62 | 62 | -1 | -0.94 | firefox | tracemonkey | 3 | Page Request | 100 | 1 | 1 | 0 | 17.05 | firefox | tracemonkey | 3 | Rendering | 100 | 61 | 61 | -1 | -1.11 | firefox | tracemonkey | 4 | Overall | 100 | 56 | 58 | 2 | 3.41 | firefox | tracemonkey | 4 | Page Request | 100 | 1 | 1 | 0 | 15.31 | firefox | tracemonkey | 4 | Rendering | 100 | 55 | 57 | 2 | 3.23 | firefox | tracemonkey | 5 | Overall | 100 | 73 | 71 | -2 | -2.28 | firefox | tracemonkey | 5 | Page Request | 100 | 2 | 2 | 0 | 12.20 | firefox | tracemonkey | 5 | Rendering | 100 | 71 | 69 | -2 | -2.69 | firefox | tracemonkey | 6 | Overall | 100 | 85 | 69 | -16 | -18.73 | faster firefox | tracemonkey | 6 | Page Request | 100 | 2 | 2 | 0 | -9.90 | firefox | tracemonkey | 6 | Rendering | 100 | 83 | 67 | -16 | -18.97 | faster firefox | tracemonkey | 7 | Overall | 100 | 65 | 64 | 0 | -0.37 | firefox | tracemonkey | 7 | Page Request | 100 | 1 | 1 | 0 | -11.94 | firefox | tracemonkey | 7 | Rendering | 100 | 63 | 63 | 0 | -0.05 | firefox | tracemonkey | 8 | Overall | 100 | 53 | 54 | 1 | 2.04 | firefox | tracemonkey | 8 | Page Request | 100 | 1 | 1 | 0 | 17.02 | firefox | tracemonkey | 8 | Rendering | 100 | 52 | 53 | 1 | 1.82 | firefox | tracemonkey | 9 | Overall | 100 | 79 | 73 | -6 | -7.86 | faster firefox | tracemonkey | 9 | Page Request | 100 | 2 | 2 | 0 | -15.14 | firefox | tracemonkey | 9 | Rendering | 100 | 77 | 71 | -6 | -7.86 | faster firefox | tracemonkey | 10 | Overall | 100 | 545 | 519 | -27 | -4.86 | faster firefox | tracemonkey | 10 | Page Request | 100 | 14 | 13 | 0 | -3.56 | firefox | tracemonkey | 10 | Rendering | 100 | 532 | 506 | -26 | -4.90 | faster firefox | tracemonkey | 11 | Overall | 100 | 42 | 41 | -1 | -2.50 | firefox | tracemonkey | 11 | Page Request | 100 | 1 | 1 | 0 | -27.42 | faster firefox | tracemonkey | 11 | Rendering | 100 | 41 | 40 | -1 | -1.75 | firefox | tracemonkey | 12 | Overall | 100 | 350 | 332 | -18 | -5.16 | faster firefox | tracemonkey | 12 | Page Request | 100 | 3 | 3 | 0 | -5.17 | firefox | tracemonkey | 12 | Rendering | 100 | 347 | 329 | -18 | -5.15 | faster firefox | tracemonkey | 13 | Overall | 100 | 31 | 31 | 0 | 0.52 | firefox | tracemonkey | 13 | Page Request | 100 | 1 | 1 | 0 | 4.95 | firefox | tracemonkey | 13 | Rendering | 100 | 30 | 30 | 0 | 0.20 | ```
Snuffleupagus
force-pushed
the
cache-ColorSpace-locally
branch
from
494df78 to
5c39de8
Compare
|
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/370ed04bc5bab37/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/b960dbe9f198f54/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/b960dbe9f198f54/output.txt Total script time: 25.71 mins
Image differences available at: http://54.67.70.0:8877/b960dbe9f198f54/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/370ed04bc5bab37/output.txt Total script time: 28.55 mins
Image differences available at: http://54.215.176.217:8877/370ed04bc5bab37/reftest-analyzer.html#web=eq.log |
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/1bf5786e6f8a681/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/1bf5786e6f8a681/output.txt Total script time: 3.35 mins Published |
| @@ -1198,6 +1221,7 @@ var PartialEvaluator = (function PartialEvaluatorClosure() { | |||
| var xref = this.xref; | |||
| let parsingText = false; | |||
| const localImageCache = new LocalImageCache(); | |||
| const localColorSpaceCache = new LocalImageCache(); | |||
There was a problem hiding this comment.
Just a note for possible follow-ups: if we start using this cache more often, we might want to consider renaming it since it's now used for more than just image caching.
There was a problem hiding this comment.
You're absolutely correct here; this was just me being lazy :-)
We could easily just have a base class, with Image/ColorSpace/etc. ones extending it as needed. If we want to cache by reference as well, which may help for the SMask code-path, then we probably need slightly different set methods anyway...
There was a problem hiding this comment.
This is now being fixed, among other things, in PR #12012 :-)
| @@ -1468,12 +1502,22 @@ var PartialEvaluator = (function PartialEvaluatorClosure() { | |||
| }) | |||
| ); | |||
| return; | |||
| case OPS.setStrokeColorSpace: | |||
| } | |||
| case OPS.setStrokeColorSpace: { | |||
There was a problem hiding this comment.
The curly braces here and above are a bit inconsistent with the other cases and don't really seem necessary, but I can imagine it looks a bit better to group the code. I don't mind either way, but perhaps we might consider making it more consistent later.
There was a problem hiding this comment.
That was actually necessary because of how scopes work with switch-statements, please see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/switch#Block-scope_variables_within_switch_statements
timvandermeij
approved these changes
Jun 14, 2020
|
Looks good; thank you! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By caching parsed
ColorSpaces, we thus don't need to re-parse the same data over and over which saves CPU cycles and reduces peak memory usage. (Obviously persistent memory usage may increase a tiny bit, but since the caching is done perPartialEvaluator.getOperatorListinvocation and given thatColorSpaceinstances generally hold very little data this shouldn't be much of an issue.)Furthermore, by caching
ColorSpaces we can also lookup the already parsed ones synchronously during theOperatorListbuilding, instead of having to defer to the event loop/microtask queue since the parsing is done asynchronously (such that error handling is easier).Possible future improvements:
ColorSpacesused inPatterns andImages.ColorSpaces by reference as well, in addition to only by name, assuming that there's documents where that would be beneficial and that it's not too difficult to implement.ColorSpaces globally as well.Given that we've never, until now, been doing any caching of parsed
ColorSpaces and that even using a simple name-only local cache helps tremendously in pathological cases, I purposely decided against complicating the implementation too much initially.Also, compared to parsing of
Images, simply creating aColorSpaceinstance isn't that expensive (hence I'd be somewhat surprised if adding a global cache would help much).Please note: Whenever you're dealing with documents which are "slow", there's usually a certain level of subjectivity involved with regards to what's deemed acceptable performance.
Over the years a number of patches have improved the performance of issue #2504 incrementally, but with the changes in this patch alone we've now (on average) reduced the rendering time by 50 % which I'd thus say is good enough to call the issue fixed :-)
Fixes #2504
This patch was tested using:
tracemonkeyPDF file, which was included mostly to show that "normal" documents aren't negatively affected by these changes.ColorSpaces.with the following manifest file:
which gave the following results when comparing this patch against the
masterbranch: