|
|
DescriptionCache SkTypeface for alternative family name
The details in crbug.com/424082, especially in comment 30.
On Linux, Views/HarfBuzz makes many calls to CraeteSkTypeface,
and by default its FontList is derived from the user's desktop
setting (Gnome etc.). If the default UI font has multiple
family names (canonical one and locale-sensitive ones),
the system might feed the localized one in the FontList.
In that case, FontConfigTypeface::LegacyCreateTypeface() is
called with the localized family name but skia caches with
its primary family name as a key for it.
This caused many fontconfig matchFamilyName() calls, which
takes ~5ms on z620 for each. Sometimes, e.g. rendering
omnibox suggestion popup, this interface is called thousands
of times, and resulted in very sluggish UI. Other than that,
every UI element in Chrome browser (non-web content) which
uses the same font make Chrome slower.
In my specific case, I used
https://store1.adobe.com/cfusion/store/html/index.cfm?store=OLS-JP&event=displayFontPackage&code=1967
which has "源ノ角ゴシック JP" as localized family name while
it has preferred family name "Source Han Sans JP".
This may cause a bit more usage of cache, but such font is
not so many and it should not hurt much.
This also fixes a ChromeOS specific situation of "ui-sans".
For "ui-sans", matchFamilyName() outputs the family name as
"Noto UI Sans" which already exists in the cache, therefore
looking up with this font name will always cause cache-miss
and lookup. We will have to put the cache regardless whether
the second cache hits or not. See crbug.com/444894#c10.
BUG=chromium:424082, chromium:444894
Patch Set 1 #Patch Set 2 : add a comment #
Total comments: 2
Patch Set 3 : apply mukai's change from https://codereview.chromium.org/844333004/ #Patch Set 4 : rebase #Messages
Total messages: 27 (3 generated)
reed@google.com changed reviewers: + bungeman@google.com, reed@google.com
Hi, could you review this? I'm not sure we should do any modification to Legacy*() interface, but this has severe penalty if this problem is hit.
mukai@chromium.org changed reviewers: + mukai@chromium.org
Could you also add chromium:444894 to the bug line? https://codereview.chromium.org/838743002/diff/20001/src/ports/SkFontHost_fon... File src/ports/SkFontHost_fontconfig.cpp (right): https://codereview.chromium.org/838743002/diff/20001/src/ports/SkFontHost_fon... src/ports/SkFontHost_fontconfig.cpp:115: if (face) { You'll have to put the face_orig to the cache in this case. If outFamilyName exactly matches to an existing one in the cache, face_orig never comes to the cache. See my comments on crbug.com/444894#c10 and https://codereview.chromium.org/844333004
https://codereview.chromium.org/838743002/diff/20001/src/ports/SkFontHost_fon... File src/ports/SkFontHost_fontconfig.cpp (right): https://codereview.chromium.org/838743002/diff/20001/src/ports/SkFontHost_fon... src/ports/SkFontHost_fontconfig.cpp:115: if (face) { On 2015/01/12 23:50:43, Jun Mukai wrote: > You'll have to put the face_orig to the cache in this case. If outFamilyName > exactly matches to an existing one in the cache, face_orig never comes to the > cache. > See my comments on crbug.com/444894#c10 and > https://codereview.chromium.org/844333004 Thanks for your further investigation! Applied your change into this CL (and the description as well).
So the general idea behind the SkTypefaceCache is that requests that map to the same logical typeface map as often as possible to the same SkTypeface instance. This means fewer SkTypeface objects, and more importantly more hits to the glyph cache (since the SkTypeface id is part of the SkScalerContextRec / cache key). The implementation of this is in two parts: 1. Early: Cache requests, the exact same request can just map to the same typeface. Note that no back-end information is required here, this can be done based on request information alone. Also, this is a { request -> SkTypeface }, so multiple requests may point to the same SkTypeface 2. Late: For each cached typeface, ask the back-end if this request will map to that typeface. Note that this is something like calling a factory with its own cache. The back-end could create a native typeface from the request, and compare it to the native part of the typeface in the cache. Note that this is an iteration over [ SkTypeface ], we don't want duplicates here. In some sense this means take the current request, cannonicalize it, then either add the cannonicalized version to the cache, or return the one that's already in the cache. Currently, these two parts are 'merged' since the second half covers the first half (each implementation needs to do the 'simple' check). The issue is that we're essentially only doing (2). Also, we don't store the entire request, just the style, so (1) can't even be done completely. Basically (1) has the downside of false negatives, but the upside of skipping the (potentially expensive) cannonicalization step. I'll see about updating the SkTypefaceCache to actually do (1).
On 2015/01/13 15:30:52, bungeman1 wrote: > So the general idea behind the SkTypefaceCache is that requests that map to the > same logical typeface map as often as possible to the same SkTypeface instance. > This means fewer SkTypeface objects, and more importantly more hits to the glyph > cache (since the SkTypeface id is part of the SkScalerContextRec / cache key). > > The implementation of this is in two parts: > 1. Early: Cache requests, the exact same request can just map to the same > typeface. > Note that no back-end information is required here, this can be done based on > request information alone. > Also, this is a { request -> SkTypeface }, so multiple requests may point to > the same SkTypeface > > 2. Late: For each cached typeface, ask the back-end if this request will map to > that typeface. > Note that this is something like calling a factory with its own cache. > The back-end could create a native typeface from the request, and compare it > to the native part of the typeface in the cache. > Note that this is an iteration over [ SkTypeface ], we don't want duplicates > here. > In some sense this means take the current request, cannonicalize it, then > either add the cannonicalized version to the cache, or return the one that's > already in the cache. > > Currently, these two parts are 'merged' since the second half covers the first > half (each implementation needs to do the 'simple' check). The issue is that > we're essentially only doing (2). Also, we don't store the entire request, just > the style, so (1) can't even be done completely. Basically (1) has the downside > of false negatives, but the upside of skipping the (potentially expensive) > cannonicalization step. > > I'll see about updating the SkTypefaceCache to actually do (1). IIUC, do you recommend that I should rewrite the current change so that for the case when requested family name and canonicalized family name differs, we should create only one SkTypeface instance rather than two, and make it discoverable from both names?
On 2015/01/14 07:40:31, Takayoshi Kochi wrote: > On 2015/01/13 15:30:52, bungeman1 wrote: > > So the general idea behind the SkTypefaceCache is that requests that map to > the > > same logical typeface map as often as possible to the same SkTypeface > instance. > > This means fewer SkTypeface objects, and more importantly more hits to the > glyph > > cache (since the SkTypeface id is part of the SkScalerContextRec / cache key). > > > > The implementation of this is in two parts: > > 1. Early: Cache requests, the exact same request can just map to the same > > typeface. > > Note that no back-end information is required here, this can be done based > on > > request information alone. > > Also, this is a { request -> SkTypeface }, so multiple requests may point > to > > the same SkTypeface > > > > 2. Late: For each cached typeface, ask the back-end if this request will map > to > > that typeface. > > Note that this is something like calling a factory with its own cache. > > The back-end could create a native typeface from the request, and compare > it > > to the native part of the typeface in the cache. > > Note that this is an iteration over [ SkTypeface ], we don't want > duplicates > > here. > > In some sense this means take the current request, cannonicalize it, then > > either add the cannonicalized version to the cache, or return the one that's > > already in the cache. > > > > Currently, these two parts are 'merged' since the second half covers the first > > half (each implementation needs to do the 'simple' check). The issue is that > > we're essentially only doing (2). Also, we don't store the entire request, > just > > the style, so (1) can't even be done completely. Basically (1) has the > downside > > of false negatives, but the upside of skipping the (potentially expensive) > > cannonicalization step. > > > > I'll see about updating the SkTypefaceCache to actually do (1). > > IIUC, do you recommend that I should rewrite the current change so that > for the case when requested family name and canonicalized family name differs, > we should create only one SkTypeface instance rather than two, and make > it discoverable from both names? Hmm, does it boil down to the following TODO in SkTypefaceCache.h? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s... /* TODO * Provide std way to cache name+requestedStyle aliases to the same typeface. * * The current mechanism ends up create a diff typeface for each one, even if * they map to the same internal obj (e.g. CTFontRef on the mac) */
> > Hmm, does it boil down to the following TODO in SkTypefaceCache.h? > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s... > > /* TODO > * Provide std way to cache name+requestedStyle aliases to the same typeface. > * > * The current mechanism ends up create a diff typeface for each one, even if > * they map to the same internal obj (e.g. CTFontRef on the mac) > */ Essentially, yes. We currently don't quite use the cached requested style on most back-ends either, unfortunately. I'm going to take a look today and tomorrow looking at replacing SkTypefaceCache with an SkResourceCache implementation which can do what we want.
On 2015/01/15 15:01:58, bungeman1 wrote: > > > > Hmm, does it boil down to the following TODO in SkTypefaceCache.h? > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s... > > > > /* TODO > > * Provide std way to cache name+requestedStyle aliases to the same typeface. > > * > > * The current mechanism ends up create a diff typeface for each one, even if > > * they map to the same internal obj (e.g. CTFontRef on the mac) > > */ > > Essentially, yes. We currently don't quite use the cached requested style on > most back-ends either, unfortunately. I'm going to take a look today and > tomorrow looking at replacing SkTypefaceCache with an SkResourceCache > implementation which can do what we want. Thanks, then I'll wait for it so we don't duplicate the effort and avoid conflict.
On 2015/01/16 08:33:50, Takayoshi Kochi wrote: > On 2015/01/15 15:01:58, bungeman1 wrote: > > > > > > Hmm, does it boil down to the following TODO in SkTypefaceCache.h? > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s... > > > > > > /* TODO > > > * Provide std way to cache name+requestedStyle aliases to the same > typeface. > > > * > > > * The current mechanism ends up create a diff typeface for each one, even > if > > > * they map to the same internal obj (e.g. CTFontRef on the mac) > > > */ > > > > Essentially, yes. We currently don't quite use the cached requested style on > > most back-ends either, unfortunately. I'm going to take a look today and > > tomorrow looking at replacing SkTypefaceCache with an SkResourceCache > > implementation which can do what we want. > > Thanks, then I'll wait for it so we don't duplicate the effort and avoid > conflict. bungeman: any updates? Do you already have a crbug entry to track this?
On 2015/01/30 04:54:33, Takayoshi Kochi wrote: > On 2015/01/16 08:33:50, Takayoshi Kochi wrote: > > On 2015/01/15 15:01:58, bungeman1 wrote: > > > > > > > > Hmm, does it boil down to the following TODO in SkTypefaceCache.h? > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s... > > > > > > > > /* TODO > > > > * Provide std way to cache name+requestedStyle aliases to the same > > typeface. > > > > * > > > > * The current mechanism ends up create a diff typeface for each one, > even > > if > > > > * they map to the same internal obj (e.g. CTFontRef on the mac) > > > > */ > > > > > > Essentially, yes. We currently don't quite use the cached requested style on > > > most back-ends either, unfortunately. I'm going to take a look today and > > > tomorrow looking at replacing SkTypefaceCache with an SkResourceCache > > > implementation which can do what we want. > > > > Thanks, then I'll wait for it so we don't duplicate the effort and avoid > > conflict. > > bungeman: any updates? > Do you already have a crbug entry to track this? Due to several refactorings which have been put off for some time, the needed changes were more difficult that they needed to be. I've spent the past week cleaning things up, and I'll be working on this next week. I haven't forgotten about this, and this issue and related issue https://codereview.chromium.org/697383002/ are on my radar for M42.
On 2015/01/30 16:05:41, bungeman1 wrote: > On 2015/01/30 04:54:33, Takayoshi Kochi wrote: > > On 2015/01/16 08:33:50, Takayoshi Kochi wrote: > > > On 2015/01/15 15:01:58, bungeman1 wrote: > > > > > > > > > > Hmm, does it boil down to the following TODO in SkTypefaceCache.h? > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s... > > > > > > > > > > /* TODO > > > > > * Provide std way to cache name+requestedStyle aliases to the same > > > typeface. > > > > > * > > > > > * The current mechanism ends up create a diff typeface for each one, > > even > > > if > > > > > * they map to the same internal obj (e.g. CTFontRef on the mac) > > > > > */ > > > > > > > > Essentially, yes. We currently don't quite use the cached requested style > on > > > > most back-ends either, unfortunately. I'm going to take a look today and > > > > tomorrow looking at replacing SkTypefaceCache with an SkResourceCache > > > > implementation which can do what we want. > > > > > > Thanks, then I'll wait for it so we don't duplicate the effort and avoid > > > conflict. > > > > bungeman: any updates? > > Do you already have a crbug entry to track this? > > Due to several refactorings which have been put off for some time, the needed > changes were more difficult that they needed to be. I've spent the past week > cleaning things up, and I'll be working on this next week. I haven't forgotten > about this, and this issue and related issue > https://codereview.chromium.org/697383002/ are on my radar for M42. Cool, thanks!
Ping! Ben, you said M42, and time is short?
On 2015/03/16 13:39:24, tomhudson wrote: > Ping! Ben, you said M42, and time is short? Ben, is this issue fixed elsewhere already?
On 2015/04/21 06:50:01, Takayoshi Kochi wrote: > On 2015/03/16 13:39:24, tomhudson wrote: > > Ping! Ben, you said M42, and time is short? > > Ben, is this issue fixed elsewhere already? Rebased to ToT.
On 2015/07/16 03:31:23, Takayoshi Kochi wrote: > On 2015/04/21 06:50:01, Takayoshi Kochi wrote: > > On 2015/03/16 13:39:24, tomhudson wrote: > > > Ping! Ben, you said M42, and time is short? > > > > Ben, is this issue fixed elsewhere already? > > Rebased to ToT. reed1@, bungeman1@, could you please review this? We're having performance problem when testing the chrome for chromeos on linux desktop, and this patch fixes the issue. Thanks!
Is this a correct description of the situation? "NameA" --> font_file_A "NameB" --> font_file_A "NameC" --> font_file_A If so, how is this CL addressing this many-to-one mapping? 1. It creates 3 typeface objects all that reference font_file_A 2. It creates 1 typeface but with 3 aliases in the a name-cache 3. other?
On 2015/07/30 13:31:16, reed1 wrote: > Is this a correct description of the situation? > > "NameA" --> font_file_A > "NameB" --> font_file_A > "NameC" --> font_file_A > > If so, how is this CL addressing this many-to-one mapping? > > 1. It creates 3 typeface objects all that reference font_file_A > 2. It creates 1 typeface but with 3 aliases in the a name-cache > 3. other? More specifically, the description of the situation would be: "NameA" --> font_file_A "NameB" --> "NameA" And the current implementation holds only the mapping of "NameA" --> typeface A This CL adds a mapping of original query to the result, which means "NameB" --> typeface A So it would be 2; it creates 1 typeface but the multiple cache entries can point to a typeface.
On 2015/07/30 16:45:36, Jun Mukai wrote: > On 2015/07/30 13:31:16, reed1 wrote: > > Is this a correct description of the situation? > > > > "NameA" --> font_file_A > > "NameB" --> font_file_A > > "NameC" --> font_file_A > > > > If so, how is this CL addressing this many-to-one mapping? > > > > 1. It creates 3 typeface objects all that reference font_file_A > > 2. It creates 1 typeface but with 3 aliases in the a name-cache > > 3. other? > > More specifically, the description of the situation would be: > "NameA" --> font_file_A > "NameB" --> "NameA" > > And the current implementation holds only the mapping of "NameA" --> typeface A > This CL adds a mapping of original query to the result, which means "NameB" --> > typeface A > > So it would be 2; it creates 1 typeface but the multiple cache entries can point > to a typeface. As mukai-san replied, 2 - it should not waste much memory.
I'm not sure I agree that #2 doesn't waste much memory. It doesn't duplicate any memory related to the font_file itself (assuming mmap shares pages w/ multiple clients) but it completely duplicates ALL memory allocated by Skia for the cache and scalecontext, since the two typefaces will have different uniqueIDs, which is how Skia decides if it can share caches.
On 2015/07/31 13:46:13, reed1 wrote: > I'm not sure I agree that #2 doesn't waste much memory. It doesn't duplicate any > memory related to the font_file itself (assuming mmap shares pages w/ multiple > clients) but it completely duplicates ALL memory allocated by Skia for the cache > and scalecontext, since the two typefaces will have different uniqueIDs, which > is how Skia decides if it can share caches. Hmm, okay, it seems this CL needs rework. At first it could be done by tweaking FontIdentity in SkFontConfigInterface, but it seems not trivial (still should be doable, but not sure it is the best way). I don't have much time to work on this right now, feel free to assign someone in your team (Skia or Chrome OS) to continue this.
dmazzoni@chromium.org changed reviewers: + dmazzoni@chromium.org
Ping - is anyone still looking at this? In practice this patch works great for me, everything is incredibly janky without it when I run a chromeos build on desktop Linux. Thanks.
+1 to dmazzoni. I found several web pages mentioning this problem. It already affects many users. Could you apply this patch until the right change is introduced? This is way more serious than you imagine - I already encountered this problem at work and home twice.
This is done by bungeman@ at: https://codereview.chromium.org/1683883002 |