Remove unnecessary font loading when loading font fallbacks.

Remove unnecessary font loading when loading font fallbacks.

Previously, we were going through all possible fallback fonts and
testing each for existence as soon as something asked for any fallback
font. Due to how isFontPresent is implemented, it actually loads the
font. This change defers testing font existence (and loading them) until
a font for that script is actually needed. I expect this will result in
a perf gain, since I expect most pages will need only a few fallback
fonts.

In my very limited and not very rigorous testing, I observed about a 5%
improvement in warm startup (Startup.FirstWebContents.NonEmptyPaint2)
when navigating to emojipedia.org.

BUG=631258
Committed: https://crrev.com/3f489f6b953f464a1a0b5996bc79e468ba9e35e1
Cr-Commit-Position: refs/heads/master@{#410729}

[Ilya Kulshin, 2016-08-04 01:46:53]

kulshin@chromium.org changed reviewers:
+ kojii@chromium.org

[Ilya Kulshin, 2016-08-04 01:46:53]

ptal, especially the Han fallback logic, since you've been working recently in
that area.

[Ilya Kulshin, 2016-08-04 01:47:11]

The CQ bit was checked by kulshin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 01:47:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 02:48:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-04 02:48:56]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Ilya Kulshin, 2016-08-04 05:31:29]

The CQ bit was checked by kulshin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 05:31:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-08-04 06:01:53]

Thank you for working on this. I was a little concerned about this too, but when
I measured, this function only took 10ms on my z620, and I wasn't sure if that
buys additional check of "attemptedLoad" on every lookup. z620 is probably too
fast to measure on, and if you see actual improvements, that's superb.

I'm concerned that looping as many times as the number of requested scripts.
While I agree that only very few will be used, 2 is possible, 3 may be. Please
see comments inline below:

[kojii, 2016-08-04 06:02:03]

https://codereview.chromium.org/2215543002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/win/FontFallbackWin.cpp (right):

https://codereview.chromium.org/2215543002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/win/FontFallbackWin.cpp:73: bool
attemptedLoad;
How about adding:
  const UChar** candidateFamilyNames;
so that we only need to loop once, but still can lazily resolve the first
available font?

https://codereview.chromium.org/2215543002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/win/FontFallbackWin.cpp:307: if (script
!= hanScript) {
With the CL a few days ago, scriptForHan() will never return USCRIPT_HAN, so you
can replace this with DCHECK(). There's some unknown I need to understand better
for Korean, but this is the current assumption.

https://codereview.chromium.org/2215543002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/win/FontFallbackWin.cpp:309:
initializeScriptFontMapping(hanScript, hanMapping, fontManager);
Assuming this function is given "scriptFontMap" to resolve the loop issue
mentioned above, we need to do this only when
!scriptFontMap[hanScript].attemptedLoad.

But since this code runs only when !scriptFontMap[USCRIPT_HAN].attemptedLoad, I
can't imagine the case when !scriptFontMap[hanScript].attemptedLoad. Maybe
DCHECK should be fine.

[commit-bot: I haz the power, 2016-08-04 06:52:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-04 06:52:22]

Dry run: This issue passed the CQ dry run.

[Ilya Kulshin, 2016-08-06 03:20:50]

ptal.

I refactored the code as you suggested, so that now it will loop once to find
the candidate families (without loading anything) and then loop through the
candidates as-needed when the script is requested.

I ran a few more tests and confirmed my observations that this improves
performance when fallback is needed. I'm not sure when you did your testing, but
if it was before M52 you may have been running the font cache code. In that
case, there would be a penalty at startup to load *all* fonts, but subsequent
font loading would be very fast because they would already be loaded.

https://codereview.chromium.org/2215543002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/win/FontFallbackWin.cpp (right):

https://codereview.chromium.org/2215543002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/win/FontFallbackWin.cpp:73: bool
attemptedLoad;
On 2016/08/04 06:02:03, kojii wrote:
> How about adding:
>   const UChar** candidateFamilyNames;
> so that we only need to loop once, but still can lazily resolve the first
> available font?

Done.

https://codereview.chromium.org/2215543002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/win/FontFallbackWin.cpp:307: if (script
!= hanScript) {
On 2016/08/04 06:02:03, kojii wrote:
> With the CL a few days ago, scriptForHan() will never return USCRIPT_HAN, so
you
> can replace this with DCHECK(). There's some unknown I need to understand
better
> for Korean, but this is the current assumption.

Acknowledged.

https://codereview.chromium.org/2215543002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/win/FontFallbackWin.cpp:309:
initializeScriptFontMapping(hanScript, hanMapping, fontManager);
On 2016/08/04 06:02:03, kojii wrote:
> Assuming this function is given "scriptFontMap" to resolve the loop issue
> mentioned above, we need to do this only when
> !scriptFontMap[hanScript].attemptedLoad.
> 
> But since this code runs only when !scriptFontMap[USCRIPT_HAN].attemptedLoad,
I
> can't imagine the case when !scriptFontMap[hanScript].attemptedLoad. Maybe
> DCHECK should be fine.

Acknowledged.

[Ilya Kulshin, 2016-08-06 03:25:44]

The CQ bit was checked by kulshin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-06 03:25:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-06 04:41:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-06 04:41:36]

Dry run: This issue passed the CQ dry run.

[kojii, 2016-08-06 09:33:12]

kojii@chromium.org changed reviewers:
+ drott@chromium.org, eae@chromium.org

[kojii, 2016-08-06 09:33:13]

Non-owner lgtm with nit. drott/eae, PTAL.

> I ran a few more tests and confirmed my observations that this improves
performance when fallback is needed.

I tested a couple of weeks ago, but only measured the init function in warm
condition, so you tested this trade off giving a total win is great, thank you.

https://codereview.chromium.org/2215543002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/win/FontFallbackWin.cpp (right):

https://codereview.chromium.org/2215543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/win/FontFallbackWin.cpp:447: if
(!scriptMonospaceFontMap[script].familyName) {
Isn't this supposed to be:
  if (scriptMonospaceFontMap[script].candidateFamilyNames) {
In case the first attempt ended with no figure for the script?

[eae, 2016-08-08 12:59:54]

LGTM once you've addressed kojiis comment.

[Ilya Kulshin, 2016-08-08 19:57:50]

The CQ bit was checked by kulshin@chromium.org to run a CQ dry run

[Ilya Kulshin, 2016-08-08 19:58:29]

https://codereview.chromium.org/2215543002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/win/FontFallbackWin.cpp (right):

https://codereview.chromium.org/2215543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/win/FontFallbackWin.cpp:447: if
(!scriptMonospaceFontMap[script].familyName) {
On 2016/08/06 09:33:13, kojii wrote:
> Isn't this supposed to be:
>   if (scriptMonospaceFontMap[script].candidateFamilyNames) {
> In case the first attempt ended with no figure for the script?

Since there's only two monospace overrides and they do not check for existence
of the font, I think it's better to just go through the monospace list each
time. That also means we can save the memory that would otherwise be used by
|scriptMonospaceFontMap|. Except that I forgot to actually finish that refactor
and remove |scriptMonospaceFontMap|. Thanks for pointing it out.

[commit-bot: I haz the power, 2016-08-08 19:58:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Kulshin, 2016-08-08 20:01:15]

The CQ bit was checked by kulshin@chromium.org to run a CQ dry run

[Ilya Kulshin, 2016-08-08 20:01:41]

Patchset #5 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-08-08 20:02:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-08 21:25:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-08 21:25:41]

Dry run: This issue passed the CQ dry run.

[kojii, 2016-08-09 06:35:48]

lgtm

[Ilya Kulshin, 2016-08-09 17:32:41]

The CQ bit was checked by kulshin@chromium.org

[Ilya Kulshin, 2016-08-09 17:32:41]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2215543002/#ps100001
(title: "Remove scriptMonospaceFontMap")

[commit-bot: I haz the power, 2016-08-09 17:33:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 17:38:47]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-08-09 17:41:11]

Description was changed from

==========
Remove unnecessary font loading when loading font fallbacks.

Previously, we were going through all possible fallback fonts and
testing each for existence as soon as something asked for any fallback
font. Due to how isFontPresent is implemented, it actually loads the
font. This change defers testing font existence (and loading them) until
a font for that script is actually needed. I expect this will result in
a perf gain, since I expect most pages will need only a few fallback
fonts.

In my very limited and not very rigorous testing, I observed about a 5%
improvement in warm startup (Startup.FirstWebContents.NonEmptyPaint2)
when navigating to emojipedia.org.

BUG=631258
==========

to

==========
Remove unnecessary font loading when loading font fallbacks.

Previously, we were going through all possible fallback fonts and
testing each for existence as soon as something asked for any fallback
font. Due to how isFontPresent is implemented, it actually loads the
font. This change defers testing font existence (and loading them) until
a font for that script is actually needed. I expect this will result in
a perf gain, since I expect most pages will need only a few fallback
fonts.

In my very limited and not very rigorous testing, I observed about a 5%
improvement in warm startup (Startup.FirstWebContents.NonEmptyPaint2)
when navigating to emojipedia.org.

BUG=631258

Committed: https://crrev.com/3f489f6b953f464a1a0b5996bc79e468ba9e35e1
Cr-Commit-Position: refs/heads/master@{#410729}
==========

[commit-bot: I haz the power, 2016-08-09 17:41:12]

Patchset 5 (id:??) landed as
https://crrev.com/3f489f6b953f464a1a0b5996bc79e468ba9e35e1
Cr-Commit-Position: refs/heads/master@{#410729}