Avoid allocation in SkGlyphCache::unicharToGlyph() |
||||||||||||||||||
Issue descriptionAs part of identifying redundant allocations in Chrome, I found that in some cases SkGlyphCache::unicharToGlyph() (via SkGlyphCache::getCharGlyphRec()) allocates 150 KiB of identical data (75 * 2 KiB). See detailed info here: https://docs.google.com/spreadsheets/d/1HfHKXy4q-ytZ-nwPay8YYyVSn1o-X9I7cWzYlygvtwQ/edit#gid=191544548 As I understand the following happens: * unicharToGlyph() calls getCharGlyphRec() * getCharGlyphRec() sees that fPackedUnicharIDToPackedGlyphID is NULL, allocates it, fills with {SkGlyph::kImpossibleID, 0}, and returns a pointer to {SkGlyph::kImpossibleID, 0} item * unicharToGlyph() sees that returned fPackedUnicharID (which is SkGlyph::kImpossibleID) doesn't match, and takes fScalerContext->charToGlyphID() path I.e. unicharToGlyph() doesn't really use result of getCharGlyphRec() in this case, making the allocation inside getCharGlyphRec() redundant. I'm proposing either: 1. (Hacky) In unicharToGlyph() check that fPackedUnicharIDToPackedGlyphID is NULL, and if it is, don't call getCharGlyphRec(), and take fScalerContext->charToGlyphID() path. 2. (Less hacky) Add an argument to getCharGlyphRec() that would make it skip fPackedUnicharIDToPackedGlyphID allocation and just return NULL. Use that argument in unicharToGlyph() and handle NULL return value. Don't use that argument in other places (lookupByChar). (More info about the effort is here: https://groups.google.com/a/chromium.org/forum/#!msg/project-trim/EiKXwnbTvbo/0z-Y5Kg1CgAJ)
,
Aug 16 2016
This looks like lazy initialization of a buffer that will be needed if we ever fetch any valid glyphs from the font, but Ben would know better.
,
Aug 16 2016
I think herb wrote most of the code in question.
,
Aug 16 2016
This situation only happens when only unicharToGlyph is called and no other calls (like getting the advances, metrics or bits) is called. This strategy anticipates additional glyph calls being made. I don't know how often Chrome only asks for the glyphID without ever drawing or measuring, but if this is common, then the memory is allocated needlessly. I can investigate filling in the unichar to glyph mapping more aggressively if chrome only ever needs to convert unichar to glyph IDs.
,
Aug 16 2016
Yes, that's pretty common - I saw those 75 identical entries all the time. Bug description has a reference to a sheet with a backtrace, and the first non-skia frame is blink::FontCache::getFamilyNameForCharacter(), see https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp?l=75 The whole backtrace if dealing with font fallbacks it seems, and I think magic number 75 is the number of fallback fonts it considers (on my particular test phone). And it doesn't seem the code ever draws of measures those chars, it just wants glyphID.
,
Aug 17 2016
The following revision refers to this bug: https://skia.googlesource.com/skia.git/+/1ae57b37bd718418f564e13675b5cfebe499f7db commit 1ae57b37bd718418f564e13675b5cfebe499f7db Author: herb <herb@google.com> Date: Wed Aug 17 20:51:54 2016 Eagerly update the unichar to glyphID mapping. In addition, some small cleanups. BUG=chromium:635005 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2249063005 Review-Url: https://codereview.chromium.org/2249063005 [modify] https://crrev.com/1ae57b37bd718418f564e13675b5cfebe499f7db/src/core/SkGlyphCache.cpp [modify] https://crrev.com/1ae57b37bd718418f564e13675b5cfebe499f7db/src/core/SkGlyphCache.h
,
Aug 17 2016
Uhm, so the change above fixes performance, but not memory? Is there a metric that was improved with that fix?
,
Aug 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dfba9df5e13bb6519ab8052dc812ef94f1e12211 commit dfba9df5e13bb6519ab8052dc812ef94f1e12211 Author: skia-deps-roller <skia-deps-roller@chromium.org> Date: Thu Aug 18 01:10:53 2016 Roll src/third_party/skia/ 0e7bddfb3..70f5251cc (8 commits). https://chromium.googlesource.com/skia.git/+log/0e7bddfb340a..70f5251cc591 $ git log 0e7bddfb3..70f5251cc --date=short --no-merges --format='%ad %ae %s' 2016-08-17 senorblanco Fix assert caused by floating point error in tessellating path renderer. 2016-08-17 fmalita [SVGDom] Add support for assorted absolute units 2016-08-17 halcanary SkPDF: cache metrics once. 2016-08-17 brianosman Add alphaType() to SkImage 2016-08-17 anmittal Fix BUILD.gn for arch other then x86 and x64 2016-08-17 bsalomon Fix computation of clipped src rect when tiling bitmaps in SkGpuDevice GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2252913004 2016-08-17 csmartdalton Reduce window rectangles cap to 8 2016-08-17 herb Eagerly update the unichar to glyphID mapping. BUG=635005 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel TBR=robertphillips@google.com Review-Url: https://codereview.chromium.org/2251033004 Cr-Commit-Position: refs/heads/master@{#412712} [modify] https://crrev.com/dfba9df5e13bb6519ab8052dc812ef94f1e12211/DEPS
,
Aug 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dfba9df5e13bb6519ab8052dc812ef94f1e12211 commit dfba9df5e13bb6519ab8052dc812ef94f1e12211 Author: skia-deps-roller <skia-deps-roller@chromium.org> Date: Thu Aug 18 01:10:53 2016 Roll src/third_party/skia/ 0e7bddfb3..70f5251cc (8 commits). https://chromium.googlesource.com/skia.git/+log/0e7bddfb340a..70f5251cc591 $ git log 0e7bddfb3..70f5251cc --date=short --no-merges --format='%ad %ae %s' 2016-08-17 senorblanco Fix assert caused by floating point error in tessellating path renderer. 2016-08-17 fmalita [SVGDom] Add support for assorted absolute units 2016-08-17 halcanary SkPDF: cache metrics once. 2016-08-17 brianosman Add alphaType() to SkImage 2016-08-17 anmittal Fix BUILD.gn for arch other then x86 and x64 2016-08-17 bsalomon Fix computation of clipped src rect when tiling bitmaps in SkGpuDevice GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2252913004 2016-08-17 csmartdalton Reduce window rectangles cap to 8 2016-08-17 herb Eagerly update the unichar to glyphID mapping. BUG=635005 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel TBR=robertphillips@google.com Review-Url: https://codereview.chromium.org/2251033004 Cr-Commit-Position: refs/heads/master@{#412712} [modify] https://crrev.com/dfba9df5e13bb6519ab8052dc812ef94f1e12211/DEPS
,
Aug 22 2016
The buffer in question is still being allocated, but now it's updated with resolved glyph id mapping, hopefully increasing performance.
,
Oct 6 2016
I still can see the duplication when visiting theverge.com. I turned out that sometimes Blink tries to render characters not present in fallback typefaces, filling their SkGlyphCache::fPackedUnicharIDToPackedGlyphID with identical values (fPackedUnicharID = <character>, fPackedGlyphID = 0). Here is the stack trace where fPackedUnicharIDToPackedGlyphID is allocated: ... blink::LayoutBlockFlow::layoutBlock blink::LayoutBlockFlow::layoutBlockFlow blink::LayoutBlockFlow::layoutInlineChildren blink::LayoutBlockFlow::layoutRunsAndFloats blink::LayoutBlockFlow::layoutRunsAndFloatsInRange blink::LineBreaker::nextLineBreak blink::BreakingContext::handleText blink::LayoutText::width blink::LayoutText::computePreferredLogicalWidths blink::Font::width blink::Font::floatWidthForComplexText blink::CachingWordShaper::width blink::CachingWordShapeIterator::next blink::CachingWordShapeIterator::shapeToEndIndex blink::CachingWordShapeIterator::shapeWord blink::CachingWordShapeIterator::shapeWordWithoutSpacing blink::HarfBuzzShaper::shapeResult blink::FontFallbackIterator::next blink::FontFallbackIterator::next blink::FontFallbackIterator::uniqueSystemFontForHint blink::FontCache::fallbackFontForCharacter blink::FontCache::getFamilyNameForCharacter SkFontMgr_Android::onMatchFamilyStyleCharacter SkFontMgr_Android::find_family_style_character SkPaint::textToGlyphs SkGlyphCache::unicharToGlyph I instrumented find_family_style_character() to print how many typefaces it tried (diff is attached). Visiting CNN.com gives this: find_family_style_character(character=F133): failure after probing 61 typefaces find_family_style_character(character=F133): failure after probing 14 typefaces find_family_style_character(character=F121): failure after probing 61 typefaces find_family_style_character(character=F121): failure after probing 14 typefaces find_family_style_character(character=F144): failure after probing 61 typefaces find_family_style_character(character=F144): failure after probing 14 typefaces find_family_style_character(character=65B0): success after probing 54 typefaces find_family_style_character(character=5E74): success after probing 54 typefaces find_family_style_character(character=597D): success after probing 54 typefaces find_family_style_character(character=65B0): success after probing 54 typefaces find_family_style_character(character=5E74): success after probing 54 typefaces find_family_style_character(character=597D): success after probing 54 typefaces find_family_style_character(character=65B0): success after probing 54 typefaces find_family_style_character(character=5E74): success after probing 54 typefaces find_family_style_character(character=597D): success after probing 54 typefaces find_family_style_character(character=65B0): success after probing 54 typefaces find_family_style_character(character=5E74): success after probing 54 typefaces find_family_style_character(character=597D): success after probing 54 typefaces find_family_style_character(character=65B0): success after probing 54 typefaces find_family_style_character(character=5E74): success after probing 54 typefaces find_family_style_character(character=597D): success after probing 54 typefaces find_family_style_character(character=65B0): success after probing 54 typefaces find_family_style_character(character=5E74): success after probing 54 typefaces find_family_style_character(character=597D): success after probing 54 typefaces find_family_style_character(character=65B0): success after probing 54 typefaces find_family_style_character(character=5E74): success after probing 54 typefaces find_family_style_character(character=597D): success after probing 54 typefaces At first all 75 SkGlyphCache instances were identical, but then we found some chars in 55th typeface, so we ended up with 74 identical SkGlyphCaches. Similar picture when visiting theverge.com: find_family_style_character(character=E805): failure after probing 61 typefaces find_family_style_character(character=E805): failure after probing 14 typefaces find_family_style_character(character=E804): failure after probing 61 typefaces find_family_style_character(character=E804): failure after probing 14 typefaces find_family_style_character(character=E81C): failure after probing 61 typefaces find_family_style_character(character=E81C): failure after probing 14 typefaces find_family_style_character(character=E826): failure after probing 61 typefaces find_family_style_character(character=E826): failure after probing 14 typefaces find_family_style_character(character=E824): failure after probing 61 typefaces find_family_style_character(character=E824): failure after probing 14 typefaces find_family_style_character(character=E82C): failure after probing 61 typefaces find_family_style_character(character=E82C): failure after probing 14 typefaces find_family_style_character(character=E823): failure after probing 61 typefaces find_family_style_character(character=E823): failure after probing 14 typefaces find_family_style_character(character=E802): failure after probing 61 typefaces find_family_style_character(character=E802): failure after probing 14 typefaces And washingtonpost.com: find_family_style_character(character=F0A2): failure after probing 61 typefaces find_family_style_character(character=F0A2): failure after probing 14 typefaces find_family_style_character(character=F030): failure after probing 61 typefaces find_family_style_character(character=F030): failure after probing 14 typefaces find_family_style_character(character=F0A9): failure after probing 61 typefaces find_family_style_character(character=F0A9): failure after probing 14 typefaces find_family_style_character(character=F0C9): failure after probing 61 typefaces find_family_style_character(character=F0C9): failure after probing 14 typefaces find_family_style_character(character=F013): failure after probing 61 typefaces find_family_style_character(character=F013): failure after probing 14 typefaces find_family_style_character(character=F002): failure after probing 61 typefaces find_family_style_character(character=F002): failure after probing 14 typefaces find_family_style_character(character=E60E): failure after probing 61 typefaces find_family_style_character(character=E60E): failure after probing 14 typefaces find_family_style_character(character=E615): failure after probing 61 typefaces find_family_style_character(character=E615): failure after probing 14 typefaces So, can we avoid creating SkGlyphCache instances for typefaces that don't have characters we ask for? Or somehow exclude classes of typefaces from the probing? I imagine that the process of probing 75 typefaces is also slow, so there is probably a perf gain to that.
,
Oct 24 2016
Guys, any thoughts on this?
,
Oct 24 2016
The implementation of SkFontMgr_Android::find_family_style_character is pretty bad. At some point in the distant past it was attempted to change it, but that caused issues. I'd have to take a look to see what they were. This really isn't an issue with the SkGlyphCache, which is working the way it should. Note however that fixing SkFontMgr_Android::find_family_style_character would probably end up using just as much if not more memory. The SkGlyphCache has a budget (it's managed as an LRU and tracks how much memory it's using), so just because there are a number of similar items here, they aren't going to grow without bound. As a result all changes in this area will affect performance, but are unlikely to actually affect overall memory usage.
,
Oct 24 2016
Thanks for the explanation! So even if you fix find_family_style_character(), it will still go through SkGlyphCache? I wonder if it's possible to expose SkGlyphCache_Globals instance so we could report its utilization in Chrome's "skia" memory dump provider? I can see that current limits are 2 MiB / 2048 caches, and I'm curious what is real-world usage of that. (Also, it seems that size of fPackedUnicharIDToPackedGlyphID is not added to SkGlyphCache::fMemoryUsed.)
,
Nov 17 2016
Any update on this? Do we need any blink-side changes to avoid the extra copy?
,
Aug 2 2017
Available bug with an owner? How about assigned bug with an owner?
,
Aug 22 2017
,
Aug 28 2017
,
May 18 2018
,
May 18 2018
I'm currently working on this. |
||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||
Comment 1 Deleted