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

Issue 342823005: Cache character fallback information in WebFontInfo::fallbackFontForChar (Closed)

Created:
6 years, 6 months ago by Dominik Röttsches
Modified:
6 years, 5 months ago
CC:
blink-reviews
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

Cache character fallback information in WebFontInfo::fallbackFontForChar Original patch by Eric Seidel, after a discussion with Behdad. I rebased it on top of my font fallback fix in r176765, without which this caching logic would regress (see crbug.com/272006). Quoting Eric's initial changelog entry: "The old code used to query FontConfig with the char and locale for every time Blink needed a fallback font for a character. The new code only queries FontConfig once for each locale, saves the resulting FcFontSet. When we need to know the correct font for a char, we walk the fonts in the set asking each if they have a glyph for that character. The existing logic had a bunch of code to work around bugs where FontConfig would return invalid fonts. To avoid walking invalid fonts for each lookup we create a Vector of FontCache objects at CachedFontSet creation time and only walk valid CachedFonts at lookup time." For the list of page_cycler.intl_ja_zh sites, I locally get the following speedups in warm_times results: (full data attached to the bug): 3.8397612488521577 www.baidu.com 2090.750000ms 544.500000ms 3.447672499174645 www.yahoo.co.jp 2610.750000ms 757.250000ms 3.268197920237687 cn.yahoo.com 7150.000000ms 2187.750000ms 2.5949379270522566 zol.com.cn 6741.000000ms 2597.750000ms 2.2926255669410383 goo.ne.jp 3412.000000ms 1488.250000ms 2.2606811489527194 www.sina.com.cn 10172.500000ms 4499.750000ms 2.218289901154351 www.qq.com 8023.000000ms 3616.750000ms 2.1029045643153528 2ch.net 633.500000ms 301.250000ms 1.9500301628795496 kakaku.com 4848.750000ms 2486.500000ms 1.844571008634944 fc2.com_ja 2510.000000ms 1360.750000ms 1.8037104522113632 www.google.com.hk 2722.250000ms 1509.250000ms 1.79902755267423 jugem.jp 4162.500000ms 2313.750000ms 1.778485665278118 www.taobao.com_index_global.ph 3629.000000ms 2040.500000ms 1.7573505120581434 ruten.com.tw 5319.500000ms 3027.000000ms 1.7528028651510432 dtiblog.com 5628.250000ms 3211.000000ms 1.5707825244286522 udn.com_NEWS_mainpage.shtm 4862.750000ms 3095.750000ms 1.517787031528852 hatena.ne.jp 3189.250000ms 2101.250000ms 1.5078812316715542 www.amazon.co.j 2056.750000ms 1364.000000ms 1.4083924349881796 mixi.jp 1191.500000ms 846.000000ms BUG=266214 R=behdad,eae,eseidel Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178446

Patch Set 1 #

Patch Set 2 : Initial attempt of a performance test #

Patch Set 3 : Test case moved to separate CL #

Patch Set 4 : Rebaselining missing glyph cases #

Patch Set 5 : Removing default substitution of FcFontSort Params #

Patch Set 6 : Adding test #

Patch Set 7 : Calling substitution again, let's see test failures on the bot now #

Patch Set 8 : fallbackFontForChar updated to support empty locale #

Patch Set 9 : Unit test removed, TestExpecations untouched #

Patch Set 10 : Rebaselined, null locale handling #

Patch Set 11 : Moving static local variable into function. #

Patch Set 12 : Rebaselines for files with missing glyph (new code doesn't just return Arial if no glyph is found) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+218 lines, -70 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +12 lines, -0 lines 0 comments Download
M Source/platform/exported/linux/WebFontInfo.cpp View 1 2 3 4 5 6 7 8 9 10 2 chunks +206 lines, -70 lines 0 comments Download

Messages

Total messages: 33 (0 generated)
Dominik Röttsches
6 years, 6 months ago (2014-06-24 15:43:26 UTC) #1
eae
It would be great if we could add a test for this.
6 years, 6 months ago (2014-06-24 15:53:51 UTC) #2
Dominik Röttsches
On 2014/06/24 15:53:51, eae wrote: > It would be great if we could add a ...
6 years, 6 months ago (2014-06-24 16:10:54 UTC) #3
eae
On 2014/06/24 16:10:54, Dominik Röttsches wrote: > On 2014/06/24 15:53:51, eae wrote: > > It ...
6 years, 6 months ago (2014-06-24 16:12:39 UTC) #4
Dominik Röttsches
As one step of preparation, I'll enable the fallback layout test https://codereview.chromium.org/356733003/ Looking at the ...
6 years, 6 months ago (2014-06-25 14:33:11 UTC) #5
Dominik Röttsches
I developed a performance test which shows a very clear speedup when you run content ...
6 years, 5 months ago (2014-06-30 19:47:48 UTC) #6
Dominik Röttsches
The difference between running it headless and in GUI mode is that DRT uses a ...
6 years, 5 months ago (2014-07-01 20:31:38 UTC) #7
behdad_google
lgtm
6 years, 5 months ago (2014-07-01 20:43:11 UTC) #8
eae
On 2014/07/01 20:43:11, behdad_google wrote: > lgtm LGTM
6 years, 5 months ago (2014-07-01 20:49:48 UTC) #9
eseidel
This was a huge win on the perf bots when we did this the first ...
6 years, 5 months ago (2014-07-01 21:17:46 UTC) #10
eae
On 2014/07/01 21:17:46, eseidel wrote: > This was a huge win on the perf bots ...
6 years, 5 months ago (2014-07-01 21:20:18 UTC) #11
Dominik Röttsches
The CQ bit was checked by dominik.rottsches@intel.com
6 years, 5 months ago (2014-07-03 07:12:17 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dominik.rottsches@intel.com/342823005/40001
6 years, 5 months ago (2014-07-03 07:12:41 UTC) #13
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink ...
6 years, 5 months ago (2014-07-03 08:50:44 UTC) #14
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 5 months ago (2014-07-03 09:16:52 UTC) #15
commit-bot: I haz the power
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/14611)
6 years, 5 months ago (2014-07-03 09:16:53 UTC) #16
Dominik Röttsches
The old code (in DRT mode) did return Arial for the sesame emphasis mark, even ...
6 years, 5 months ago (2014-07-03 14:12:17 UTC) #17
Dominik Röttsches
I think I figured out the root cause. If we substitute values in our FcFontSort ...
6 years, 5 months ago (2014-07-03 17:57:53 UTC) #18
Dominik Röttsches
:-( I guess I should have followed Emil's recommendation to write a test earlier. Running ...
6 years, 5 months ago (2014-07-04 13:41:42 UTC) #19
behdad_google
Ok let me study this a bit and get back to you. On 2014/07/04 13:41:42, ...
6 years, 5 months ago (2014-07-04 15:44:32 UTC) #20
behdad_google
On 2014/07/04 13:41:42, Dominik Röttsches wrote: > :-( I guess I should have followed Emil's ...
6 years, 5 months ago (2014-07-04 18:15:14 UTC) #21
behdad_google
Failure analysis here: https://code.google.com/p/chromium/issues/detail?id=392724
6 years, 5 months ago (2014-07-10 04:16:54 UTC) #22
Dominik Röttsches
Thanks, Behdad, for the analysis! I updated the code to support empty locales, as suggest ...
6 years, 5 months ago (2014-07-10 13:30:44 UTC) #23
eseidel
I doubt we want to be calling out to the real FontConfig during a unittest, ...
6 years, 5 months ago (2014-07-10 16:59:37 UTC) #24
Dominik Röttsches
On 2014/07/10 16:59:37, eseidel wrote: > I doubt we want to be calling out to ...
6 years, 5 months ago (2014-07-10 19:50:26 UTC) #25
eseidel
Yeah, sounds like the kind of test which is very useful during writing, but probably ...
6 years, 5 months ago (2014-07-10 20:11:48 UTC) #26
Dominik Röttsches
On 2014/07/10 20:11:48, eseidel wrote: > Yeah, sounds like the kind of test which is ...
6 years, 5 months ago (2014-07-10 20:16:23 UTC) #27
eae
The CQ bit was checked by eae@chromium.org
6 years, 5 months ago (2014-07-18 14:58:12 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dominik.rottsches@intel.com/342823005/240001
6 years, 5 months ago (2014-07-18 14:58:35 UTC) #29
commit-bot: I haz the power
Change committed as 178446
6 years, 5 months ago (2014-07-18 15:01:45 UTC) #30
behdad_google
On 2014/07/18 15:01:45, I haz the power (commit-bot) wrote: > Change committed as 178446 Nice!
6 years, 5 months ago (2014-07-18 15:06:36 UTC) #31
Dominik Röttsches
Looking good: Character Fallback perf test from ~1300 to ~215ms. https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=linux-release&tests=blink_perf%2FLayout_character_fallback&checked=core Page Cycler cold times ...
6 years, 5 months ago (2014-07-19 07:18:12 UTC) #32
eae
6 years, 5 months ago (2014-07-19 17:05:34 UTC) #33
Message was sent while issue was closed.
Nice!

Powered by Google App Engine
This is Rietveld 408576698