New issue
Advanced search Search tips

Issue 791475 link

Starred by 2 users
Status: Fixed
Owner:
drott@chromium.org
Closed: Dec 2017
Cc:
behdad@chromium.org
kojii@chromium.org
e...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

HasSpaceInLigaturesOrKerning is performing redundant work

Project Member Reported by drott@chromium.org, Dec 4 2017 
The font analysis performed in HasSpaceInLigaturesOrKerning is a function on FontPlatformData and is performed a very large number of times during incoming shaping calls into CachingWordShaper and word cache segmentation. 

The actual font table analysis is performed on hb_face and is size independent. The result of this analysis should be stored in HarfBuzzFace and computed only once per unique SkTypeface / font blob.

This is the likely trigger / playing into the regressions we observed in  issue 782220 , when we changed the hb_set implementation.

Comment 1 by drott@chromium.org, Dec 4 2017 

On the other hand, https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/fonts/Font.cpp?rcl=b36a00bfde5f4d4dde4b98362a00dffcca717236&l=402 the information about whether a Font object can shape by word is computed and stored per Font - and counting the calls to HasSpaceInLigaturesOrKerning does not show an excessive number of calls.
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 7 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b809ea4814644cce7c9219ff55f2f18b18d56351

commit b809ea4814644cce7c9219ff55f2f18b18d56351
Author: Dominik Röttsches <drott@chromium.org>
Date: Thu Dec 07 10:31:39 2017

Move HasSpaceInLigaturesOrKerning to HarfBuzzFace

Previously a set if bechmark stories triggered the expensive glyph
collection in GSUB and GPOS way too many times. This was found while
investigating and hb_set performance  issue 781794 . browse:news:cnn
triggered the call and GSUB/GPOS scanning method 1395 times, and was
reduced to two. browse:news:hackernews 33 times reduced to 4,
browse:social:twitter 58 times reduced to 4,
browse:social:twitter_infinite_scroll 38 times reduced to 4 as well.

We only need to do this work once per font face, independent of size -
whereas previously it was once per Font object, which, depending on
layout of the page, triggers much more frequently.

Test: FontPlatformDataTest
Bug:  791475 
Change-Id: I15757abab2037677d5ea4c640ac26ab99da4649c
Reviewed-on: https://chromium-review.googlesource.com/809106
Commit-Queue: Dominik Röttsches <drott@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522394}
[modify] https://crrev.com/b809ea4814644cce7c9219ff55f2f18b18d56351/third_party/WebKit/Source/platform/fonts/FontPlatformData.cpp
[modify] https://crrev.com/b809ea4814644cce7c9219ff55f2f18b18d56351/third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp
[modify] https://crrev.com/b809ea4814644cce7c9219ff55f2f18b18d56351/third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.h
[modify] https://crrev.com/b809ea4814644cce7c9219ff55f2f18b18d56351/third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFontCache.h

Comment 3 by drott@chromium.org, Dec 7 2017

Status: Fixed (was: Assigned)
►

Sign in to add a comment