Improve performance of reused patterns. #13770
Conversation
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @brendandahl received. Current queue size: 0 Live output at: http://54.67.70.0:8877/d5d9b646423c11d/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @brendandahl received. Current queue size: 0 Live output at: http://3.101.106.178:8877/3d0d28086e2d419/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/d5d9b646423c11d/output.txt Total script time: 32.70 mins
Image differences available at: http://54.67.70.0:8877/d5d9b646423c11d/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://3.101.106.178:8877/3d0d28086e2d419/output.txt Total script time: 38.24 mins
Image differences available at: http://3.101.106.178:8877/3d0d28086e2d419/reftest-analyzer.html#web=eq.log |
src/core/evaluator.js
Outdated
| const patternIR = shadingFill.getIR(); | ||
| id = `pattern_${this.idFactory.createObjId()}`; | ||
| localPatternCache.set(keyObj, id); | ||
| this.handler.send("obj", [id, this.pageIndex, "Pattern", patternIR], []); |
There was a problem hiding this comment.
The third argument is the transfers, and since there isn't any here I'd suggest just removing the empty Array.
| localColorSpaceCache, | ||
| localPatternCache, | ||
| }); | ||
| operatorList.addOp(fn, ["Shading", objId]); |
There was a problem hiding this comment.
To prevent any future issues, I believe that you also want to add operatorList.addDependency(objId); before this line since you're no longer sending the data inline here.
There was a problem hiding this comment.
The pattern should always be sent before the id is created, so a dependency pause shouldn't be needed. If it isn't sent first there will be an error on the main thread when we try to fetch the object (I did this accidentally).
| localColorSpaceCache, | ||
| localPatternCache, | ||
| }); | ||
| args = [patternId]; |
There was a problem hiding this comment.
As mentioned above, you probably want to add a operatorList.addDependency(patternId); before this line.
src/core/evaluator.js
Outdated
| @@ -1513,6 +1542,7 @@ class PartialEvaluator { | |||
| const localColorSpaceCache = new LocalColorSpaceCache(); | |||
| const localGStateCache = new LocalGStateCache(); | |||
| const localTilingPatternCache = new LocalTilingPatternCache(); | |||
| const localPatternCache = new Map(); | |||
There was a problem hiding this comment.
Would it perhaps help to implement a more "proper" cache here, see the existing ones in https://github.com/mozilla/pdf.js/blob/master/src/core/image_utils.js, since that may help improve overall caching given you could then cache by e.g. actual Refs instead?
(This may, or may not, help all that much for the particular data in question, but I wanted to ask.)
There was a problem hiding this comment.
Also, perhaps localShadingPatternCache instead to provide a slightly clearer distinction between this new cache and the existing one just above?
There was a problem hiding this comment.
I started out using the localTilingPatternCache, but found some issues with it and and was planning to file some bugs. The patterns can't be cached by name alone since they could be in different resources dictionaries with duplicate names (issue6737.pdf has an example, but with radial gradients). There also seemed to be a number of pdfs where the pattern/shading dictionary is inline and caching by ref can't be used.
As for perf, I don't think caching by ref should speed it up much, the shading/pattern dict should be in the xref's cache map, so it should be a pretty fast lookup.
There was a problem hiding this comment.
I started out using the localTilingPatternCache,
You probably would've needed to implement a new and separate cache, rather than simply re-using the existing localTilingPatternCache for the Shadings-case that this patch is about.
but found some issues with it and and was planning to file some bugs.
What sort of bugs did you encounter?
There also seemed to be a number of pdfs where the pattern/shading dictionary is inline and caching by ref can't be used.
As for perf, I don't think caching by ref should speed it up much, the shading/pattern dict should be in the xref's cache map, so it should be a pretty fast lookup.
I wasn't really worried about the Dict lookups themselves, since as you say the data is being cached, but was mostly wondering if caching by Ref could possibly have improved the hit/miss ratio in the cache. It doesn't sound like that'd been the case here though :-)
There was a problem hiding this comment.
Tiling patterns and shadings can both be patterns, so I should have been able to re-use that cache. I filed #13780 to further explain the issue.
src/display/pattern_helper.js
Outdated
| } | ||
| applyBoundingBox(tmpCtx, this._bbox); | ||
|
|
||
| createGradient(ctx) { |
There was a problem hiding this comment.
Nit: Perhaps make this "private" with a leading underscore in the name?
src/core/evaluator.js
Outdated
| @@ -1513,6 +1542,7 @@ class PartialEvaluator { | |||
| const localColorSpaceCache = new LocalColorSpaceCache(); | |||
| const localGStateCache = new LocalGStateCache(); | |||
| const localTilingPatternCache = new LocalTilingPatternCache(); | |||
| const localPatternCache = new Map(); | |||
There was a problem hiding this comment.
Also, perhaps localShadingPatternCache instead to provide a slightly clearer distinction between this new cache and the existing one just above?
src/core/evaluator.js
Outdated
| @@ -1314,6 +1314,35 @@ class PartialEvaluator { | |||
| }); | |||
| } | |||
|
|
|||
| parsePattern({ | |||
There was a problem hiding this comment.
Nit: Maybe parseShading instead.
brendandahl
force-pushed
the
cache-pattern
branch
from
a891946
to
12cd0bd
Compare
Bug 1721218 has a shading pattern that was used thousands of times. To improve performance of this PDF: - add a cache for patterns in the evaluator and only send the IR form once to the main thread (this also makes caching in canvas easier) - cache the created canvas radial/axial patterns - for shading fill radial/axial use the pattern directly instead of creating temporary canvas
brendandahl
force-pushed
the
cache-pattern
branch
from
12cd0bd
to
da1af02
Compare
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/fb299277f7cd504/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://3.101.106.178:8877/299d4a071f8e95d/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/fb299277f7cd504/output.txt Total script time: 32.80 mins
Image differences available at: http://54.67.70.0:8877/fb299277f7cd504/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://3.101.106.178:8877/299d4a071f8e95d/output.txt Total script time: 38.09 mins
Image differences available at: http://3.101.106.178:8877/299d4a071f8e95d/reftest-analyzer.html#web=eq.log |
|
/botio makeref |
From: Bot.io (Linux m4)ReceivedCommand cmd_makeref from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/202b8b75c272bce/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_makeref from @Snuffleupagus received. Current queue size: 1 Live output at: http://3.101.106.178:8877/ed3299def79d5c6/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/202b8b75c272bce/output.txt Total script time: 29.64 mins
|
From: Bot.io (Windows)SuccessFull output at http://3.101.106.178:8877/ed3299def79d5c6/output.txt Total script time: 35.35 mins
|
Bug 1721218 has a shading pattern that was used thousands of times.
To improve performance of this PDF:
to the main thread (this also makes caching in canvas easier)
canvas