Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(129)

Issue 838743002: Cache SkTypeface for alternative family name (Closed)

Created:
5 years, 11 months ago by kochi
Modified:
4 years, 10 months ago
CC:
reviews_skia.org
Base URL:
https://chromium.googlesource.com/skia.git@master
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

Cache 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+15 lines, -5 lines) Patch
M src/ports/SkFontHost_fontconfig.cpp View 1 2 3 2 chunks +15 lines, -5 lines 0 comments Download

Messages

Total messages: 27 (3 generated)
reed1
5 years, 11 months ago (2015-01-07 14:46:12 UTC) #2
kochi
Hi, could you review this? I'm not sure we should do any modification to Legacy*() ...
5 years, 11 months ago (2015-01-08 01:55:53 UTC) #3
Jun Mukai
Could you also add chromium:444894 to the bug line? https://codereview.chromium.org/838743002/diff/20001/src/ports/SkFontHost_fontconfig.cpp File src/ports/SkFontHost_fontconfig.cpp (right): https://codereview.chromium.org/838743002/diff/20001/src/ports/SkFontHost_fontconfig.cpp#newcode115 src/ports/SkFontHost_fontconfig.cpp:115: ...
5 years, 11 months ago (2015-01-12 23:50:43 UTC) #5
kochi
https://codereview.chromium.org/838743002/diff/20001/src/ports/SkFontHost_fontconfig.cpp File src/ports/SkFontHost_fontconfig.cpp (right): https://codereview.chromium.org/838743002/diff/20001/src/ports/SkFontHost_fontconfig.cpp#newcode115 src/ports/SkFontHost_fontconfig.cpp:115: if (face) { On 2015/01/12 23:50:43, Jun Mukai wrote: ...
5 years, 11 months ago (2015-01-13 04:25:01 UTC) #6
bungeman-skia
So the general idea behind the SkTypefaceCache is that requests that map to the same ...
5 years, 11 months ago (2015-01-13 15:30:52 UTC) #7
kochi
On 2015/01/13 15:30:52, bungeman1 wrote: > So the general idea behind the SkTypefaceCache is that ...
5 years, 11 months ago (2015-01-14 07:40:31 UTC) #8
kochi
On 2015/01/14 07:40:31, Takayoshi Kochi wrote: > On 2015/01/13 15:30:52, bungeman1 wrote: > > So ...
5 years, 11 months ago (2015-01-15 08:17:17 UTC) #9
bungeman-skia
> > Hmm, does it boil down to the following TODO in SkTypefaceCache.h? > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/src/core/SkTypefaceCache.h&q=file:SkTypefaceCache.h&sq=package:chromium&l=17 ...
5 years, 11 months ago (2015-01-15 15:01:58 UTC) #10
kochi
On 2015/01/15 15:01:58, bungeman1 wrote: > > > > Hmm, does it boil down to ...
5 years, 11 months ago (2015-01-16 08:33:50 UTC) #11
kochi
On 2015/01/16 08:33:50, Takayoshi Kochi wrote: > On 2015/01/15 15:01:58, bungeman1 wrote: > > > ...
5 years, 10 months ago (2015-01-30 04:54:33 UTC) #12
bungeman-skia
On 2015/01/30 04:54:33, Takayoshi Kochi wrote: > On 2015/01/16 08:33:50, Takayoshi Kochi wrote: > > ...
5 years, 10 months ago (2015-01-30 16:05:41 UTC) #13
kochi
On 2015/01/30 16:05:41, bungeman1 wrote: > On 2015/01/30 04:54:33, Takayoshi Kochi wrote: > > On ...
5 years, 10 months ago (2015-02-04 07:53:21 UTC) #14
tomhudson
Ping! Ben, you said M42, and time is short?
5 years, 9 months ago (2015-03-16 13:39:24 UTC) #15
kochi
On 2015/03/16 13:39:24, tomhudson wrote: > Ping! Ben, you said M42, and time is short? ...
5 years, 8 months ago (2015-04-21 06:50:01 UTC) #16
kochi
On 2015/04/21 06:50:01, Takayoshi Kochi wrote: > On 2015/03/16 13:39:24, tomhudson wrote: > > Ping! ...
5 years, 5 months ago (2015-07-16 03:31:23 UTC) #17
oshima
On 2015/07/16 03:31:23, Takayoshi Kochi wrote: > On 2015/04/21 06:50:01, Takayoshi Kochi wrote: > > ...
5 years, 4 months ago (2015-07-30 01:35:25 UTC) #18
reed1
Is this a correct description of the situation? "NameA" --> font_file_A "NameB" --> font_file_A "NameC" ...
5 years, 4 months ago (2015-07-30 13:31:16 UTC) #19
Jun Mukai
On 2015/07/30 13:31:16, reed1 wrote: > Is this a correct description of the situation? > ...
5 years, 4 months ago (2015-07-30 16:45:36 UTC) #20
kochi
On 2015/07/30 16:45:36, Jun Mukai wrote: > On 2015/07/30 13:31:16, reed1 wrote: > > Is ...
5 years, 4 months ago (2015-07-31 06:28:47 UTC) #21
reed1
I'm not sure I agree that #2 doesn't waste much memory. It doesn't duplicate any ...
5 years, 4 months ago (2015-07-31 13:46:13 UTC) #22
kochi
On 2015/07/31 13:46:13, reed1 wrote: > I'm not sure I agree that #2 doesn't waste ...
5 years, 4 months ago (2015-08-03 05:39:20 UTC) #23
dmazzoni
Ping - is anyone still looking at this? In practice this patch works great for ...
5 years, 1 month ago (2015-11-18 23:42:45 UTC) #25
gotom
+1 to dmazzoni. I found several web pages mentioning this problem. It already affects many ...
4 years, 10 months ago (2016-01-29 06:21:16 UTC) #26
kochi
4 years, 10 months ago (2016-02-22 10:10:39 UTC) #27
This is done by bungeman@ at:
https://codereview.chromium.org/1683883002

Powered by Google App Engine
This is Rietveld 408576698