Introduce FontCache content scaling

Introduce FontCache content scaling

FontPlatformData stored in FontCache has size-independent backing font
data on non-Mac platform, and can be safely reused for different font
sizes by scaling.
This CL modifies FontCache to reuse a FontPlatformData to different size
of font, so that Blink's layout phase requires smaller number of expensive
synchronous IPCs.

This reduces the first layout time of text-heavy CJK sites on Linux.
E.g. reduces layout time of http://b.hatena.ne. jp/ from 740ms to 515ms,
and https://ja-jp.facebook.com/legal/terms from 453ms to 428ms.

BUG=608650
Committed: https://crrev.com/d9ea08ed34d022152362130ae95f55fbd814a3af
Cr-Commit-Position: refs/heads/master@{#395022}

[tzik, 2016-05-10 09:24:26]

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

[commit-bot: I haz the power, 2016-05-10 09:24:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962263002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962263002/1

[tzik, 2016-05-10 09:25:58]

Description was changed from

==========
FontCache content scaling

TODO:
 - Write description.
 - Implement scaleFontPlatformData
 - Measure performance.

BUG=
==========

to

==========
FontCache content scaling

wip

TODO:
 - Write description.
 - Implement scaleFontPlatformData
 - Measure performance.

BUG=
==========

[commit-bot: I haz the power, 2016-05-10 11:05:11]

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

[commit-bot: I haz the power, 2016-05-10 11:05:11]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tzik, 2016-05-10 13:41:00]

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

[commit-bot: I haz the power, 2016-05-10 13:41:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962263002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962263002/20001

[commit-bot: I haz the power, 2016-05-10 14:36:13]

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

[commit-bot: I haz the power, 2016-05-10 14:36:14]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tzik, 2016-05-10 14:42:41]

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

[commit-bot: I haz the power, 2016-05-10 14:42:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962263002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962263002/40001

[commit-bot: I haz the power, 2016-05-10 16:01:42]

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

[commit-bot: I haz the power, 2016-05-10 16:01:43]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tzik, 2016-05-17 08:00:58]

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

[commit-bot: I haz the power, 2016-05-17 08:01:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962263002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962263002/60001

[commit-bot: I haz the power, 2016-05-17 09:51:54]

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

[commit-bot: I haz the power, 2016-05-17 09:51:55]

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

[tzik, 2016-05-17 13:02:03]

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

[commit-bot: I haz the power, 2016-05-17 13:02:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962263002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962263002/80001

[tzik, 2016-05-17 13:53:41]

Description was changed from

==========
FontCache content scaling

wip

TODO:
 - Write description.
 - Implement scaleFontPlatformData
 - Measure performance.

BUG=
==========

to

==========
Introduce FontCache content scaling

FontPlatformData stored in FontCache has size-independent backing font
data on non-Mac platform, and can be safely reused for different font
sizes by scaling.
This CL modifies FontCache to reuse a FontPlatformData to different size
of font, so that Blink's layout phase requires smaller number of expensive
synchronous IPCs.

BUG=608650
==========

[tzik, 2016-05-17 13:53:41]

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

[tzik, 2016-05-17 13:55:09]

PTAL

[tzik, 2016-05-17 14:12:13]

Description was changed from

==========
Introduce FontCache content scaling

FontPlatformData stored in FontCache has size-independent backing font
data on non-Mac platform, and can be safely reused for different font
sizes by scaling.
This CL modifies FontCache to reuse a FontPlatformData to different size
of font, so that Blink's layout phase requires smaller number of expensive
synchronous IPCs.

BUG=608650
==========

to

==========
Introduce FontCache content scaling

FontPlatformData stored in FontCache has size-independent backing font
data on non-Mac platform, and can be safely reused for different font
sizes by scaling.
This CL modifies FontCache to reuse a FontPlatformData to different size
of font, so that Blink's layout phase requires smaller number of expensive
synchronous IPCs.

This reduces the first layout time of text-heavy CJK sites on Linux.
E.g. reduces layout time of http://b.hatena.ne. jp/ from 740ms to 515ms,
and https://ja-jp.facebook.com/legal/terms from 453 to 428.

BUG=608650
==========

[eae, 2016-05-17 14:42:55]

https://codereview.chromium.org/1962263002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right):

https://codereview.chromium.org/1962263002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontCache.cpp:70: typedef
HashMap<unsigned, OwnPtr<FontPlatformData>> SizedFontPlatformDataSet;
Long term we should probably try to divorce the size from FontPlatformData
entierly but in the interim this seems like a perfectly fine step. Thank's for
doing this!

https://codereview.chromium.org/1962263002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontCache.cpp:116: auto addResult =
sizedFonts->add(size ? size * FontCacheKey::precisionMultiplier() : -2,
nullptr);
-2?

https://codereview.chromium.org/1962263002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontCache.cpp:121: else
How about using sizedFonts->begin()->value.get() directly here and getting rid
of the anotherSize variable (that's only used here). Alternatively, move the
declaration of anotherSize down here from line 115.

https://codereview.chromium.org/1962263002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontCache.cpp:153: return nullptr;
When would this be null?

[tzik, 2016-05-17 14:56:19]

https://codereview.chromium.org/1962263002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right):

https://codereview.chromium.org/1962263002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontCache.cpp:116: auto addResult =
sizedFonts->add(size ? size * FontCacheKey::precisionMultiplier() : -2,
nullptr);
On 2016/05/17 14:42:55, eae wrote:
> -2?

Added a comment here. 0 and -1 are reserved for HashMap key.

https://codereview.chromium.org/1962263002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontCache.cpp:121: else
On 2016/05/17 14:42:55, eae wrote:
> How about using sizedFonts->begin()->value.get() directly here and getting rid
> of the anotherSize variable (that's only used here). Alternatively, move the
> declaration of anotherSize down here from line 115.

Hmm, since we are adding new entry into sizedFonts, begin() may point the entry
we just added. Added a comment..

https://codereview.chromium.org/1962263002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontCache.cpp:153: return nullptr;
On 2016/05/17 14:42:55, eae wrote:
> When would this be null?

This can be null if the first createFontPlatformData for the font returned null.
I.e. for an unavailable font on the system.

[eae, 2016-05-17 15:07:02]

On 2016/05/17 14:56:19, tzik wrote:
>
https://codereview.chromium.org/1962263002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right):
> 
>
https://codereview.chromium.org/1962263002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/fonts/FontCache.cpp:116: auto addResult =
> sizedFonts->add(size ? size * FontCacheKey::precisionMultiplier() : -2,
> nullptr);
> On 2016/05/17 14:42:55, eae wrote:
> > -2?
> 
> Added a comment here. 0 and -1 are reserved for HashMap key.

Thanks

> 
>
https://codereview.chromium.org/1962263002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/fonts/FontCache.cpp:121: else
> On 2016/05/17 14:42:55, eae wrote:
> > How about using sizedFonts->begin()->value.get() directly here and getting
rid
> > of the anotherSize variable (that's only used here). Alternatively, move the
> > declaration of anotherSize down here from line 115.
> 
> Hmm, since we are adding new entry into sizedFonts, begin() may point the
entry
> we just added. Added a comment..
> 
>
https://codereview.chromium.org/1962263002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/fonts/FontCache.cpp:153: return nullptr;
> On 2016/05/17 14:42:55, eae wrote:
> > When would this be null?
> 
> This can be null if the first createFontPlatformData for the font returned
null.
> I.e. for an unavailable font on the system.

Could we check for that at the callsite instead?

[tzik, 2016-05-17 15:54:32]

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

[commit-bot: I haz the power, 2016-05-17 15:54:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962263002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962263002/120001

[tzik, 2016-05-17 16:02:46]

On 2016/05/17 15:07:02, eae wrote:
> On 2016/05/17 14:56:19, tzik wrote:
> >
>
https://codereview.chromium.org/1962263002/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1962263002/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/platform/fonts/FontCache.cpp:116: auto addResult =
> > sizedFonts->add(size ? size * FontCacheKey::precisionMultiplier() : -2,
> > nullptr);
> > On 2016/05/17 14:42:55, eae wrote:
> > > -2?
> > 
> > Added a comment here. 0 and -1 are reserved for HashMap key.
> 
> Thanks
> 
> > 
> >
>
https://codereview.chromium.org/1962263002/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/platform/fonts/FontCache.cpp:121: else
> > On 2016/05/17 14:42:55, eae wrote:
> > > How about using sizedFonts->begin()->value.get() directly here and getting
> rid
> > > of the anotherSize variable (that's only used here). Alternatively, move
the
> > > declaration of anotherSize down here from line 115.
> > 
> > Hmm, since we are adding new entry into sizedFonts, begin() may point the
> entry
> > we just added. Added a comment..
> > 
> >
>
https://codereview.chromium.org/1962263002/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/platform/fonts/FontCache.cpp:153: return nullptr;
> > On 2016/05/17 14:42:55, eae wrote:
> > > When would this be null?
> > 
> > This can be null if the first createFontPlatformData for the font returned
> null.
> > I.e. for an unavailable font on the system.
> 
> Could we check for that at the callsite instead?

OK, updated.
This slightly change the behavior on Mac: if a font is unavailable for a size,
the new code assumes the font is also unavailable on all size.

[eae, 2016-05-17 16:27:43]

LGTM

[commit-bot: I haz the power, 2016-05-17 16:49:57]

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

[commit-bot: I haz the power, 2016-05-17 16:49:58]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[kouhei (in TOK), 2016-05-18 02:31:05]

kouhei@chromium.org changed reviewers:
+ kouhei@chromium.org

[kouhei (in TOK), 2016-05-18 02:31:06]

Please measure the impact of CL under finch so that you receive credit :)
e.g.) Something like 95% enable / 5% control is OK as we are confident that this
has positive impact.

Add to CL description:
- Before this CL font was queried to the backend even if the previous query of
the same FontDescription with different font size returned null. This CL
modifies the FontCache so that it can cache that "the backend doesn't have any
font for given FontDescription, regardless of font size".

https://codereview.chromium.org/1962263002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right):

https://codereview.chromium.org/1962263002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/FontCache.cpp:121: unsigned roundedSize
= size ? size * FontCacheKey::precisionMultiplier() : -2;
-2 should be defined as "static const unsigned" or something to avoid magic
numbers.

[drott, 2016-05-18 07:22:48]

tzik, thanks for the CL! Glad to see we're making progress on this. I don't
object to landing it as is, but I would have one suggestion to possibly simplify
the CL.

https://codereview.chromium.org/1962263002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right):

https://codereview.chromium.org/1962263002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/FontCache.cpp:71: typedef
HashMap<FontCacheKey, SizedFontPlatformDataSet, FontCacheKeyHash,
FontCacheKeyTraits> FontPlatformDataCache;
Do we need to keep the rescaled sizes in a cache as well? It seems like the
scale operation is quite cheap, mostly copying the FontPlatformData object. I
wonder whether we can simplify this CL by not keeping track of the rescaled
versions, and keeping only the "one size fits all" FontPlatformData in the
cache.

[drott, 2016-05-18 07:26:20]

PS: (I mean this along the lines of eae@'s earlier statement: "Long term we
should probably try to divorce the size from FontPlatformData entierly but in
the interim this seems like a perfectly fine step.")

[tzik, 2016-05-18 07:26:53]

https://codereview.chromium.org/1962263002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right):

https://codereview.chromium.org/1962263002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/FontCache.cpp:71: typedef
HashMap<FontCacheKey, SizedFontPlatformDataSet, FontCacheKeyHash,
FontCacheKeyTraits> FontPlatformDataCache;
On 2016/05/18 07:22:48, drott wrote:
> Do we need to keep the rescaled sizes in a cache as well? It seems like the
> scale operation is quite cheap, mostly copying the FontPlatformData object. I
> wonder whether we can simplify this CL by not keeping track of the rescaled
> versions, and keeping only the "one size fits all" FontPlatformData in the
> cache.

It's not so cheap actually on Linux. The scaling ctor of FontPlatformData calls
FontRenderStyle::querySystem, and it issues another synchronous IPC, which is as
expensive as the font query itself.

[drott, 2016-05-18 07:30:48]

On 2016/05/18 at 07:26:53, tzik wrote:
>
https://codereview.chromium.org/1962263002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right):
> 
>
https://codereview.chromium.org/1962263002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/fonts/FontCache.cpp:71: typedef
HashMap<FontCacheKey, SizedFontPlatformDataSet, FontCacheKeyHash,
FontCacheKeyTraits> FontPlatformDataCache;
> On 2016/05/18 07:22:48, drott wrote:
> > Do we need to keep the rescaled sizes in a cache as well? It seems like the
> > scale operation is quite cheap, mostly copying the FontPlatformData object.
I
> > wonder whether we can simplify this CL by not keeping track of the rescaled
> > versions, and keeping only the "one size fits all" FontPlatformData in the
> > cache.
> 
> It's not so cheap actually on Linux. The scaling ctor of FontPlatformData
calls FontRenderStyle::querySystem, and it issues another synchronous IPC, which
is as expensive as the font query itself.

Thanks for the explanation, LGTM. Let's aim for removing the font size from the
FontPlatformData cache.

[drott, 2016-05-18 07:31:36]

Do you know what the reason is for the mac bot failures atm?

[tzik, 2016-05-18 09:06:48]

On 2016/05/18 07:31:36, drott wrote:
> Do you know what the reason is for the mac bot failures atm?

That seems caused by a misused HashMap. Now it got green!

[tzik, 2016-05-18 13:22:22]

Patchset #10 (id:180001) has been deleted

[tzik, 2016-05-18 13:24:00]

Patchset #10 (id:200001) has been deleted

[tzik, 2016-05-19 13:52:18]

Description was changed from

==========
Introduce FontCache content scaling

FontPlatformData stored in FontCache has size-independent backing font
data on non-Mac platform, and can be safely reused for different font
sizes by scaling.
This CL modifies FontCache to reuse a FontPlatformData to different size
of font, so that Blink's layout phase requires smaller number of expensive
synchronous IPCs.

This reduces the first layout time of text-heavy CJK sites on Linux.
E.g. reduces layout time of http://b.hatena.ne. jp/ from 740ms to 515ms,
and https://ja-jp.facebook.com/legal/terms from 453 to 428.

BUG=608650
==========

to

==========
Introduce FontCache content scaling

FontPlatformData stored in FontCache has size-independent backing font
data on non-Mac platform, and can be safely reused for different font
sizes by scaling.
This CL modifies FontCache to reuse a FontPlatformData to different size
of font, so that Blink's layout phase requires smaller number of expensive
synchronous IPCs.

This reduces the first layout time of text-heavy CJK sites on Linux.
E.g. reduces layout time of http://b.hatena.ne. jp/ from 740ms to 515ms,
and https://ja-jp.facebook.com/legal/terms from 453ms to 428ms.

BUG=608650
==========

[tzik, 2016-05-19 13:56:04]

tzik@chromium.org changed reviewers:
+ isherman@chromium.org, kinuko@chromium.org

[tzik, 2016-05-19 13:56:05]

Now all test should get green.

kinuko: Could you review //chrome and //content part for Finch experiment?

isherman: Could you review //tools/metric?

[kinuko, 2016-05-19 14:36:02]

cool. finch-flag related change lgtm

https://codereview.chromium.org/1962263002/diff/260001/content/public/common/...
File content/public/common/content_features.cc (right):

https://codereview.chromium.org/1962263002/diff/260001/content/public/common/...
content/public/common/content_features.cc:39: // size of font.
nit: a font -> a cached font ?
nit: different size of font -> different sizes of the font

[Ilya Sherman, 2016-05-19 22:32:08]

histograms.xml lgtm

[kouhei (in TOK), 2016-05-20 06:57:35]

lgtm

[tzik, 2016-05-20 06:58:35]

The CQ bit was checked by tzik@chromium.org

[tzik, 2016-05-20 06:58:36]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org,
drott@chromium.org
Link to the patchset: https://codereview.chromium.org/1962263002/#ps260001
(title: "win layout test fix")

[commit-bot: I haz the power, 2016-05-20 06:58:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962263002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962263002/260001

[commit-bot: I haz the power, 2016-05-20 07:02:53]

Description was changed from

==========
Introduce FontCache content scaling

FontPlatformData stored in FontCache has size-independent backing font
data on non-Mac platform, and can be safely reused for different font
sizes by scaling.
This CL modifies FontCache to reuse a FontPlatformData to different size
of font, so that Blink's layout phase requires smaller number of expensive
synchronous IPCs.

This reduces the first layout time of text-heavy CJK sites on Linux.
E.g. reduces layout time of http://b.hatena.ne. jp/ from 740ms to 515ms,
and https://ja-jp.facebook.com/legal/terms from 453ms to 428ms.

BUG=608650
==========

to

==========
Introduce FontCache content scaling

FontPlatformData stored in FontCache has size-independent backing font
data on non-Mac platform, and can be safely reused for different font
sizes by scaling.
This CL modifies FontCache to reuse a FontPlatformData to different size
of font, so that Blink's layout phase requires smaller number of expensive
synchronous IPCs.

This reduces the first layout time of text-heavy CJK sites on Linux.
E.g. reduces layout time of http://b.hatena.ne. jp/ from 740ms to 515ms,
and https://ja-jp.facebook.com/legal/terms from 453ms to 428ms.

BUG=608650
==========

[commit-bot: I haz the power, 2016-05-20 07:02:55]

Committed patchset #12 (id:260001)

[commit-bot: I haz the power, 2016-05-20 07:04:28]

Description was changed from

==========
Introduce FontCache content scaling

FontPlatformData stored in FontCache has size-independent backing font
data on non-Mac platform, and can be safely reused for different font
sizes by scaling.
This CL modifies FontCache to reuse a FontPlatformData to different size
of font, so that Blink's layout phase requires smaller number of expensive
synchronous IPCs.

This reduces the first layout time of text-heavy CJK sites on Linux.
E.g. reduces layout time of http://b.hatena.ne. jp/ from 740ms to 515ms,
and https://ja-jp.facebook.com/legal/terms from 453ms to 428ms.

BUG=608650
==========

to

==========
Introduce FontCache content scaling

FontPlatformData stored in FontCache has size-independent backing font
data on non-Mac platform, and can be safely reused for different font
sizes by scaling.
This CL modifies FontCache to reuse a FontPlatformData to different size
of font, so that Blink's layout phase requires smaller number of expensive
synchronous IPCs.

This reduces the first layout time of text-heavy CJK sites on Linux.
E.g. reduces layout time of http://b.hatena.ne. jp/ from 740ms to 515ms,
and https://ja-jp.facebook.com/legal/terms from 453ms to 428ms.

BUG=608650

Committed: https://crrev.com/d9ea08ed34d022152362130ae95f55fbd814a3af
Cr-Commit-Position: refs/heads/master@{#395022}
==========

[commit-bot: I haz the power, 2016-05-20 07:04:30]

Patchset 12 (id:??) landed as
https://crrev.com/d9ea08ed34d022152362130ae95f55fbd814a3af
Cr-Commit-Position: refs/heads/master@{#395022}