-
Notifications
You must be signed in to change notification settings - Fork 10k
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 caching of shading patterns. (bug 1721949) #13808
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/ff14baf4af73c25/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/09e4f6a7ed401ca/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/ff14baf4af73c25/output.txt Total script time: 26.98 mins
Image differences available at: http://54.67.70.0:8877/ff14baf4af73c25/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://3.101.106.178:8877/09e4f6a7ed401ca/output.txt Total script time: 38.12 mins
Image differences available at: http://3.101.106.178:8877/09e4f6a7ed401ca/reftest-analyzer.html#web=eq.log |
/botio-linux 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/d009884031f0799/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/d009884031f0799/output.txt Total script time: 27.65 mins
Image differences available at: http://54.67.70.0:8877/d009884031f0799/reftest-analyzer.html#web=eq.log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me, with two small comments; thank you!
src/display/canvas.js
Outdated
* by last insertion. | ||
*/ | ||
class LRUCache { | ||
constructor(maxSize) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that you have a this._maxSize <= 0
check further down, you probably want to set a numerical default value here (since (undefined <= 0) === false
):
constructor(maxSize) { | |
constructor(maxSize = 0) { |
src/display/pattern_helper.js
Outdated
if (!shadingFill && this.cachedCanvasPatterns.has(this)) { | ||
pattern = this.cachedCanvasPatterns.get(this); | ||
} else { | ||
if (!shadingFill) { | ||
const tmpCanvas = owner.cachedCanvases.getCanvas( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Unless I'm completely misreading this code, it'd feel slightly easier to reason about the conditions with the following small change:
if (!shadingFill && this.cachedCanvasPatterns.has(this)) { | |
pattern = this.cachedCanvasPatterns.get(this); | |
} else { | |
if (!shadingFill) { | |
const tmpCanvas = owner.cachedCanvases.getCanvas( | |
if (!shadingFill) { | |
if (this.cachedCanvasPatterns.has(this)) { | |
pattern = this.cachedCanvasPatterns.get(this); | |
} else { | |
const tmpCanvas = owner.cachedCanvases.getCanvas( |
The PDF in bug 1721949 uses many unique pattern objects that references the same shading many times. This caused a new canvas pattern to be created and cached many times driving up memory use. To fix, I've changed the cache in the worker to key off the shading object and instead send the shading and matrix separately. While that worked well to fix the above bug, there could be PDFs that use many shading that could cause memory issues, so I've also added a LRU cache on the main thread for canvas patterns. This should prevent memory use from getting too high.
66efc87
to
c836e1f
Compare
/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/b9636a4283dfbfb/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/dd1c5e96ce9c865/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/b9636a4283dfbfb/output.txt Total script time: 33.48 mins
Image differences available at: http://54.67.70.0:8877/b9636a4283dfbfb/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://3.101.106.178:8877/dd1c5e96ce9c865/output.txt Total script time: 38.80 mins
Image differences available at: http://3.101.106.178:8877/dd1c5e96ce9c865/reftest-analyzer.html#web=eq.log |
Hmm, it looks like |
/botio makreref |
From: Bot.io (Windows)InvalidCommand not implemented: |
From: Bot.io (Linux m4)InvalidCommand not implemented: |
/botio makeref |
From: Bot.io (Linux m4)ReceivedCommand cmd_makeref from @brendandahl received. Current queue size: 1 Live output at: http://54.67.70.0:8877/f64f67868399425/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_makeref from @brendandahl received. Current queue size: 2 Live output at: http://3.101.106.178:8877/b8ea7dc718f5d1d/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/f64f67868399425/output.txt Total script time: 30.30 mins
|
From: Bot.io (Windows)SuccessFull output at http://3.101.106.178:8877/b8ea7dc718f5d1d/output.txt Total script time: 36.42 mins
|
The PDF in bug 1721949 uses many unique pattern objects
that references the same shading many times. This caused
a new canvas pattern to be created and cached many times
driving up memory use.
To fix, I've changed the cache in the worker to key off the
shading object and instead send the shading and matrix
separately. While that worked well to fix the above bug,
there could be PDFs that use many shading that could
cause memory issues, so I've also added a LRU cache
on the main thread for canvas patterns. This should prevent
memory use from getting too high.