-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Add a cache for glyphs #4453
Add a cache for glyphs #4453
Conversation
This reduces memory consumption for text heavy documents. I tested five documents and saw hit rates ranging from 97.4% to 99.8% (most of the misses are due to |width| varying even when |fontChar| matches). On two of those documents I saw improvements of 40 and 50 MiB. The patch also introduces the Glyph constructor, and renames the |unicodeChars| local variable as |unicode| for consistency with the corresponding Glyph property.
I fixed the lint error. Interestingly, I found that a lot of object allocations occur when passing data from the worker to the main thread, and glyph objects constitute most of those objects. So this cache helps twice: first, it reduces the number of objects allocated in the worker, and second, every object we don't allocate is one that we don't need to clone (the structured cloning algorithm in html5 will clone each object only once, even if it's referred to numerous times). I've confirmed the second effect via instrumentation. |
}; | ||
var fontChar = String.fromCharCode(fontCharCode); | ||
|
||
var glyph = this.glyphCache[charcode]; |
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.
If we are caching at the glyph level, does it make sense to keep the cache in charsToGlyph around still?
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.
The inner (glyph) cache reduces the number of glyph objects created. The outer (glyphs) cache reduces the number of glyphs arrays created. They're independent, and both useful.
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @brendandahl received. Current queue size: 0 Live output at: http://107.21.233.14:8877/793f71915fd5073/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @brendandahl received. Current queue size: 0 Live output at: http://107.22.172.223:8877/f5bd072e798d616/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/793f71915fd5073/output.txt Total script time: 25.07 mins
|
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/f5bd072e798d616/output.txt Total script time: 36.20 mins
|
Is this close to landing? It's a good-sized win. |
This reduces memory consumption for text heavy documents. I tested five
documents and saw hit rates ranging from 97.4% to 99.8% (most of the misses are
due to |width| varying even when |fontChar| matches). On two of those documents
I saw improvements of 40 and 50 MiB.
The patch also introduces the Glyph constructor, and renames the |unicodeChars|
local variable as |unicode| for consistency with the corresponding Glyph
property.