Unify glyph metrics to use the same Skia API

Unify glyph metrics to use the same Skia API

Use identical glyph metrics based bounds in HarfBuzz callbacks as well
as ShapeResult's glyph bounds calculation, except on Mac, due to Skia
bug https://bugs.chromium.org/p/skia/issues/detail?id=5328

* Using the same glyph bounds callback in HarfBuzz shaping and in
  ShapeResult's glyph bounds calculation enables reusing of the same
  cache in Skia, as opposed to using the cached glyph metrics based
  cache vs. using a second cache for path based metrics before.

Local benchmarking of the blink_perf.layout shows 12.44%
improvements for the character_fallback test and 1.8-5%
improvements on chapter-reflow-once, line-layout and
latin-complex-test. There are also some hits on flexbox-lots of
data and flexbox-row-nowrap. I am not 100% convinced that the
local benchmarks are accurate enough and would like to observe
the results on the bot. Overall, we should see a layout speed
improvement, perhaps even in the page cyclers.

BUG=610313
R=kojii,tzik

Committed: https://crrev.com/9e47ba024f0061ea667565451c2842f66813fee7
Cr-Commit-Position: refs/heads/master@{#407755}

[drott, 2016-05-16 14:47:15]

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

[commit-bot: I haz the power, 2016-05-16 14:47:25]

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

[drott, 2016-05-16 14:47:37]

One possible attempt for addressing tzik's findings in the bug.

[f(malita), 2016-05-16 14:59:59]

Description was changed from

==========
Unify glyph metrics to use the same Skia API and remove glyphtoBoundsMap

Removing the caches in SimpleFontData since Skia caches these further down.

BUG=610313
R=kojii,tzik
==========

to

==========
Unify glyph metrics to use the same Skia API and remove glyphtoBoundsMap

Removing the caches in SimpleFontData since Skia caches these further down.

BUG=610313
R=kojii,tzik
==========

[tzik, 2016-05-16 15:08:14]

On 2016/05/16 at 14:47:37, drott wrote:
> One possible attempt for addressing tzik's findings in the bug.

This looks good to me itself. But, does this dedup SkGlyphCache too?

[f(malita), 2016-05-16 15:08:41]

(drive-by nits)

This is great, I've been trying to get rid of the path-based glyph bounds
computation for a while (abandoned http://crrev.com/1889793006 due to
significant diffs on Mac).

We should keep an eye on perf, as Skia's cache is threadsafe and involves
synchronization.

If this lands, it will likely also solve Walter's http://crbug.com/601968.

https://codereview.chromium.org/1980913002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/fonts/skia/SkiaTextMetrics.cpp (right):

https://codereview.chromium.org/1980913002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/fonts/skia/SkiaTextMetrics.cpp:68:
m_paint->setTextEncoding(SkPaint::kGlyphID_TextEncoding);
Nit: we do this in all methods - would it make sense to set the encoding in the
ctor?

https://codereview.chromium.org/1980913002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/fonts/skia/SkiaTextMetrics.cpp:72:
m_paint->getTextWidths(&glyph, sizeof(glyph), &skWidth, &skBounds);
Nit: the bounds argument is optional, you can pass nullptr if not using
skBounds.

[tzik, 2016-05-16 15:12:24]

https://codereview.chromium.org/1980913002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/fonts/skia/SkiaTextMetrics.cpp (right):

https://codereview.chromium.org/1980913002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/fonts/skia/SkiaTextMetrics.cpp:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
ditto

https://codereview.chromium.org/1980913002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/fonts/skia/SkiaTextMetrics.h (right):

https://codereview.chromium.org/1980913002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/fonts/skia/SkiaTextMetrics.h:1: // Copyright
2015 The Chromium Authors. All rights reserved.
nit: s/2015/2016/?

[drott, 2016-05-16 15:13:43]

drott@chromium.org changed reviewers:
+ wkorman@chromium.org

[drott, 2016-05-16 15:18:50]

On 2016/05/16 at 15:08:14, tzik wrote:
> On 2016/05/16 at 14:47:37, drott wrote:
> > One possible attempt for addressing tzik's findings in the bug.
> 
> This looks good to me itself. But, does this dedup SkGlyphCache too?

I am not sure - I thought by bringing it to the same API we could avoid the
cache misses? Perhaps you could help me find out and try your analysis with this
CL added on top? How can I find out? What did you do to observe the cache misses
and cache duplication?

[commit-bot: I haz the power, 2016-05-16 15:55:16]

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

[commit-bot: I haz the power, 2016-05-16 15:55:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tzik, 2016-05-16 18:53:40]

On 2016/05/16 at 15:18:50, drott wrote:
> On 2016/05/16 at 15:08:14, tzik wrote:
> > On 2016/05/16 at 14:47:37, drott wrote:
> > > One possible attempt for addressing tzik's findings in the bug.
> > 
> > This looks good to me itself. But, does this dedup SkGlyphCache too?
> 
> I am not sure - I thought by bringing it to the same API we could avoid the
cache misses? Perhaps you could help me find out and try your analysis with this
CL added on top? How can I find out? What did you do to observe the cache misses
and cache duplication?

I think the cache miss on SkGlyphCache is due to the different SkPaint setup in
SkPaint::getTextWidths vs SkPaint::getTextPath.
SkPaint::getTextWidths calculates the glyph width with the original glyph size
(which is set up in SkCanonicalizePaint), while SkPaint::getTextPath uses a
canonical size as SkTextBaseIter::SkTextBaseIter sets up. That causes different
SkPaint descriptor value used as the cache key in SkPaint::detachCache, and
makes two separate SkGlyphCache instances.
Since this CL doesn't change the situation, I think we'll still have separate
cache instances.

For measurement, I just injected TRACE_EVENT into several place (e.g.
SkGlyphCache::getGlyphIDMetrics, blink::HarfBuzzShaper::shapeRange and
blink::HarfBuzzShaper::extractShapeResults). Then, measure it on
chrome://tracing on some CJK sites (e.g. http://b.hatena.ne.jp/ ).
If the glyph cache works as I expect, the second getGlyphIDMetrics in
extractShapeResult doesn't take long time.

[drott, 2016-05-16 19:46:31]

https://codereview.chromium.org/1980913002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/fonts/skia/SkiaTextMetrics.cpp (right):

https://codereview.chromium.org/1980913002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/fonts/skia/SkiaTextMetrics.cpp:56:
m_paint->getTextWidths(&glyph, sizeof(glyph), &skWidth, bounds);
> Since this CL doesn't change the situation, I think we'll still have separate
cache instances.

But I thought my CL does change this, since I am now using getTextWidths for
retrieving the glyph bounds in SimpleFontData, as opposed to getTextPath, as it
was before.

[kojii, 2016-05-16 23:29:31]

On 2016/05/16 at 19:46:31, drott wrote:
> > Since this CL doesn't change the situation, I think we'll still have
separate
> cache instances.
> 
> But I thought my CL does change this, since I am now using getTextWidths for
retrieving the glyph bounds in SimpleFontData, as opposed to getTextPath, as it
was before.

AFAIU, this CL should change the situation.

Currently, Skia caches for:
1. one for a size, when linearText=false (getTextWidths)
2. one for a font, when linearText=true (getTextPath)

Our try yesterday was to unify to 2 to reduce the cache as much as possible.
This CL unifies to 1, so we still cache for each size, but we should be able to
eliminate the cache for 2, unless somewhere else we call Skia function that sets
linearText=true.

tzik@, could you run your measure with this patch? Unifying to 2 should result
in less memory usage, but unifying to 1 should still save, and might solve your
original case (because we don't measure/cache twice.)

[tzik, 2016-05-17 03:15:34]

On 2016/05/16 at 23:29:31, kojii wrote:
> On 2016/05/16 at 19:46:31, drott wrote:
> > > Since this CL doesn't change the situation, I think we'll still have
separate
> > cache instances.
> > 
> > But I thought my CL does change this, since I am now using getTextWidths for
retrieving the glyph bounds in SimpleFontData, as opposed to getTextPath, as it
was before.
> 
> AFAIU, this CL should change the situation.
> 
> Currently, Skia caches for:
> 1. one for a size, when linearText=false (getTextWidths)
> 2. one for a font, when linearText=true (getTextPath)

Ah, right! I misread it. Yes, this should change the situation.

> 
> Our try yesterday was to unify to 2 to reduce the cache as much as possible.
This CL unifies to 1, so we still cache for each size, but we should be able to
eliminate the cache for 2, unless somewhere else we call Skia function that sets
linearText=true.
> 
> tzik@, could you run your measure with this patch? Unifying to 2 should result
in less memory usage, but unifying to 1 should still save, and might solve your
original case (because we don't measure/cache twice.)

Sure I will.

[tzik, 2016-05-17 07:32:33]

On 2016/05/17 03:15:34, tzik wrote:
> On 2016/05/16 at 23:29:31, kojii wrote:
> > On 2016/05/16 at 19:46:31, drott wrote:
> > > > Since this CL doesn't change the situation, I think we'll still have
> separate
> > > cache instances.
> > > 
> > > But I thought my CL does change this, since I am now using getTextWidths
for
> retrieving the glyph bounds in SimpleFontData, as opposed to getTextPath, as
it
> was before.
> > 
> > AFAIU, this CL should change the situation.
> > 
> > Currently, Skia caches for:
> > 1. one for a size, when linearText=false (getTextWidths)
> > 2. one for a font, when linearText=true (getTextPath)
> 
> Ah, right! I misread it. Yes, this should change the situation.
> 
> > 
> > Our try yesterday was to unify to 2 to reduce the cache as much as possible.
> This CL unifies to 1, so we still cache for each size, but we should be able
to
> eliminate the cache for 2, unless somewhere else we call Skia function that
sets
> linearText=true.
> > 
> > tzik@, could you run your measure with this patch? Unifying to 2 should
result
> in less memory usage, but unifying to 1 should still save, and might solve
your
> original case (because we don't measure/cache twice.)
> 
> Sure I will.

LGTM. Looks perfect to me.
This speeds up the layout performance from 1988ms to 1345ms, >30%.

[drott, 2016-05-17 08:46:59]

On 2016/05/16 at 15:08:41, fmalita wrote:
> (drive-by nits)
> 
> This is great, I've been trying to get rid of the path-based glyph bounds
computation for a while (abandoned http://crrev.com/1889793006 due to
significant diffs on Mac).

Unfortunately, I don't have the answer. The windows and Linux failure looks like
rounding changes, and perhaps something that we can just rebaseline, but in Mac
I see some redness in some "green over red" square tests, and some vertical
length differences. What did your previous investigation result in? Any idea
what's the cause for these differences? Anything inside Skia? 

tzik@, kojii@, could you help investigate those differences?

[kojii, 2016-05-17 10:22:26]

On 2016/05/17 at 08:46:59, drott wrote:
> On 2016/05/16 at 15:08:41, fmalita wrote:
> > (drive-by nits)
> > 
> > This is great, I've been trying to get rid of the path-based glyph bounds
computation for a while (abandoned http://crrev.com/1889793006 due to
significant diffs on Mac).
> 
> Unfortunately, I don't have the answer. The windows and Linux failure looks
like rounding changes, and perhaps something that we can just rebaseline, but in
Mac I see some redness in some "green over red" square tests, and some vertical
length differences. What did your previous investigation result in? Any idea
what's the cause for these differences? Anything inside Skia? 
> 
> tzik@, kojii@, could you help investigate those differences?

I missed them but yeah, "green over red" and vertical length differences look
bad. Different glyph bounds shouldn't affect height, something looks wrong.

In the mean time, could we split "remove GlyphMetricsMap" part out to a separate
patch? That shouldn't cause any pixel differences, allows us to measure perf
changes separately, and might help us to fix crbug.com/577306. Unlikely to help
tzik's original bug, but it could help to narrow down the problem.

[drott, 2016-05-19 09:09:11]

Try addressing Mac failures by removing xHeight sanitization

[drott, 2016-05-19 12:56:57]

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

[drott, 2016-05-19 12:56:58]

Alternative xHeight fixup attempt on Mac

[commit-bot: I haz the power, 2016-05-19 12:57:06]

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

[commit-bot: I haz the power, 2016-05-19 14:13:18]

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

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

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_...)

[drott, 2016-05-19 17:35:27]

Turns out that the path-based xHeight and the getTextWidths based glyph height
return rather different results, tested so far on Mac, using a quick Skia test
app.

The xHeight for the Ahem font at 8px font size, which is the same as the ascent,
as there are only "capital" blocks in Ahem, is 8 pixels for the path based
value, and 6.4pixels for the getTextWidth approach. :-( That's probably why
we're seeing the Ahem tests on Mac failing, those which are based on ex units
scaling.

[drott, 2016-05-20 09:38:29]

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

[drott, 2016-05-20 09:38:31]

new idea for mac fixup

[commit-bot: I haz the power, 2016-05-20 09:38:42]

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

[commit-bot: I haz the power, 2016-05-20 10:49:52]

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

[commit-bot: I haz the power, 2016-05-20 10:49:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[drott, 2016-05-20 14:12:12]

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

[drott, 2016-05-20 14:12:14]

keep path based metric for mac os, move encoding setting to constructor

[commit-bot: I haz the power, 2016-05-20 14:12:22]

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

[drott, 2016-05-20 14:24:30]

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

[drott, 2016-05-20 14:24:32]

bounds argument -> nullptr, copyright year

[commit-bot: I haz the power, 2016-05-20 14:24:45]

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

[commit-bot: I haz the power, 2016-05-20 15:22:55]

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

[commit-bot: I haz the power, 2016-05-20 15:22:57]

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

[drott, 2016-07-06 12:01:19]

Rebased

[drott, 2016-07-06 12:01:23]

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

[commit-bot: I haz the power, 2016-07-06 12:01:35]

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-07-06 12:55:25]

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

[commit-bot: I haz the power, 2016-07-06 12:55:27]

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

[drott, 2016-07-06 13:19:42]

Keep path based metrics on Mac

[drott, 2016-07-06 13:19:43]

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

[commit-bot: I haz the power, 2016-07-06 13:19:57]

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

[drott, 2016-07-06 13:29:41]

Description was changed from

==========
Unify glyph metrics to use the same Skia API and remove glyphtoBoundsMap

Removing the caches in SimpleFontData since Skia caches these further down.

BUG=610313
R=kojii,tzik
==========

to

==========
Unify glyph metrics to use the same Skia API and remove glyphtoBoundsMap

Removing the caches in SimpleFontData since Skia caches these
down the line.

Measuring with https://codereview.chromium.org/2123193002
applied, i.e. with the m_glyphToBoundsMap added to memory infra,
clicking through a set of pages in Wikipedia, the sum of the
glyph-to-bounds maps can grow significantly, in the same
renderer. I saw a size of 1.2MB for clicking through a few links
only on the English Wikipedia.


BUG=610313
R=kojii,tzik
==========

[commit-bot: I haz the power, 2016-07-06 14:12:49]

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

[commit-bot: I haz the power, 2016-07-06 14:12:51]

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

[drott, 2016-07-18 15:06:20]

drott@chromium.org changed reviewers:
+ pdr@chromium.org

[drott, 2016-07-18 15:21:38]

pdr@, the SVG failures look like 1-2 pixel changes to element bounding boxes.
What do you think about rebaselining those? Could you help me finding exactly
where this computation is coming from? (I am assuming indirectly from
Font::widts's FloatRect* glyphBounds out parameter (which is using a united
rectangle of each glyph's boundsForGlyph, which is changed in this CL).

[pdr., 2016-07-20 23:16:55]

On 2016/07/18 at 15:21:38, drott wrote:
> pdr@, the SVG failures look like 1-2 pixel changes to element bounding boxes.
What do you think about rebaselining those? Could you help me finding exactly
where this computation is coming from? (I am assuming indirectly from
Font::widts's FloatRect* glyphBounds out parameter (which is using a united
rectangle of each glyph's boundsForGlyph, which is changed in this CL).

Sorry about the delay, I've been out sick.

The pixel changes look good to me to rebaseline. I had a similar set of
rebaselines when switching to use harfbuzz glyphs initially.

Here's the chain of calls that ends up using these in SVG:
LayoutSVGInlineText::addMetricsFromRun -> Font::individualCharacterRanges ->
CachingWordShaper::individualCharacterRanges ->
ShapeResultBuffer::individualCharacterRanges

[drott, 2016-07-21 08:55:53]

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

[commit-bot: I haz the power, 2016-07-21 08:56:11]

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

[drott, 2016-07-21 09:00:22]

On 2016/07/20 at 23:16:55, pdr wrote:
> The pixel changes look good to me to rebaseline. I had a similar set of
rebaselines when switching to use harfbuzz glyphs initially.

Great, thank you, hope you're feeling better.

> Here's the chain of calls that ends up using these in SVG:
> LayoutSVGInlineText::addMetricsFromRun -> Font::individualCharacterRanges ->
CachingWordShaper::individualCharacterRanges ->
ShapeResultBuffer::individualCharacterRanges

Exactly what I was looking for, thanks!

[drott, 2016-07-21 09:15:24]

Description was changed from

==========
Unify glyph metrics to use the same Skia API and remove glyphtoBoundsMap

Removing the caches in SimpleFontData since Skia caches these
down the line.

Measuring with https://codereview.chromium.org/2123193002
applied, i.e. with the m_glyphToBoundsMap added to memory infra,
clicking through a set of pages in Wikipedia, the sum of the
glyph-to-bounds maps can grow significantly, in the same
renderer. I saw a size of 1.2MB for clicking through a few links
only on the English Wikipedia.


BUG=610313
R=kojii,tzik
==========

to

==========
Unify glyph metrics to use the same Skia API and remove glyphtoBoundsMap

Use identical glyph metrics based bounds in HarfBuzz callbacks as well
as ShapeResult's glyph bounds calculation, except on Mac, due to Skia
bug https://bugs.chromium.org/p/skia/issues/detail?id=5328

This CL should be a win in memory and performance on platforms except
Mac, especially on Android WebView:

* Using the same glyph bounds callback in HarfBuzz shaping and in
  ShapeResult's glyph bounds calculation enables reusing of the same
  cache in Skia, as opposed to using the cached glyph metrics based
  cache vs. using a second cache for path based metrics before.

* Removing the glyph to bounds map saves memory because of removing
  redundant caching between Blink and Skia. Anecdotal: Measuring using
  https://codereview.chromium.org/2123193002 an clicking through a few
  links in the English wikipedia in single process mode leads to the
  glyphToBoundsMap growing to a size of 1.2MB - removing this is a big
  improvement for the Android WebView, which runs in single process mode
  and accumulates size in this cache.

BUG=610313
R=kojii,tzik
==========

[drott, 2016-07-21 09:16:49]

Description was changed from

==========
Unify glyph metrics to use the same Skia API and remove glyphtoBoundsMap

Use identical glyph metrics based bounds in HarfBuzz callbacks as well
as ShapeResult's glyph bounds calculation, except on Mac, due to Skia
bug https://bugs.chromium.org/p/skia/issues/detail?id=5328

This CL should be a win in memory and performance on platforms except
Mac, especially on Android WebView:

* Using the same glyph bounds callback in HarfBuzz shaping and in
  ShapeResult's glyph bounds calculation enables reusing of the same
  cache in Skia, as opposed to using the cached glyph metrics based
  cache vs. using a second cache for path based metrics before.

* Removing the glyph to bounds map saves memory because of removing
  redundant caching between Blink and Skia. Anecdotal: Measuring using
  https://codereview.chromium.org/2123193002 an clicking through a few
  links in the English wikipedia in single process mode leads to the
  glyphToBoundsMap growing to a size of 1.2MB - removing this is a big
  improvement for the Android WebView, which runs in single process mode
  and accumulates size in this cache.

BUG=610313
R=kojii,tzik
==========

to

==========
Unify glyph metrics to use the same Skia API and remove glyphtoBoundsMap

Use identical glyph metrics based bounds in HarfBuzz callbacks as well
as ShapeResult's glyph bounds calculation, except on Mac, due to Skia
bug https://bugs.chromium.org/p/skia/issues/detail?id=5328

This CL should be a win in memory and performance on platforms except
Mac, especially on Android WebView:

* Using the same glyph bounds callback in HarfBuzz shaping and in
  ShapeResult's glyph bounds calculation enables reusing of the same
  cache in Skia, as opposed to using the cached glyph metrics based
  cache vs. using a second cache for path based metrics before.

* Removing the glyph to bounds map saves memory because of removing
  redundant caching between Blink and Skia. Anecdotal: Measuring using
  https://codereview.chromium.org/2123193002 an clicking through a few
  links in the English wikipedia in single process mode leads to the
  glyphToBoundsMap growing to a size of 1.2MB - removing this is a big
  improvement for the Android WebView, which runs in single process mode
  and accumulates space in this cache.

BUG=610313
R=kojii,tzik
==========

[commit-bot: I haz the power, 2016-07-21 10:27:25]

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

[commit-bot: I haz the power, 2016-07-21 10:27:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[drott, 2016-07-21 11:35:12]

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

[commit-bot: I haz the power, 2016-07-21 11:35:35]

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

[drott, 2016-07-21 11:40:55]

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

[commit-bot: I haz the power, 2016-07-21 13:20:49]

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

[commit-bot: I haz the power, 2016-07-21 13:20:50]

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

[drott, 2016-07-21 14:00:19]

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

[commit-bot: I haz the power, 2016-07-21 14:00:28]

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

[drott, 2016-07-21 14:50:40]

Description was changed from

==========
Unify glyph metrics to use the same Skia API and remove glyphtoBoundsMap

Use identical glyph metrics based bounds in HarfBuzz callbacks as well
as ShapeResult's glyph bounds calculation, except on Mac, due to Skia
bug https://bugs.chromium.org/p/skia/issues/detail?id=5328

This CL should be a win in memory and performance on platforms except
Mac, especially on Android WebView:

* Using the same glyph bounds callback in HarfBuzz shaping and in
  ShapeResult's glyph bounds calculation enables reusing of the same
  cache in Skia, as opposed to using the cached glyph metrics based
  cache vs. using a second cache for path based metrics before.

* Removing the glyph to bounds map saves memory because of removing
  redundant caching between Blink and Skia. Anecdotal: Measuring using
  https://codereview.chromium.org/2123193002 an clicking through a few
  links in the English wikipedia in single process mode leads to the
  glyphToBoundsMap growing to a size of 1.2MB - removing this is a big
  improvement for the Android WebView, which runs in single process mode
  and accumulates space in this cache.

BUG=610313
R=kojii,tzik
==========

to

==========
Unify glyph metrics to use the same Skia API and remove glyphtoBoundsMap

Use identical glyph metrics based bounds in HarfBuzz callbacks as well
as ShapeResult's glyph bounds calculation, except on Mac, due to Skia
bug https://bugs.chromium.org/p/skia/issues/detail?id=5328

This CL should be a win in memory and performance on platforms except
Mac, especially on Android WebView:

* Using the same glyph bounds callback in HarfBuzz shaping and in
  ShapeResult's glyph bounds calculation enables reusing of the same
  cache in Skia, as opposed to using the cached glyph metrics based
  cache vs. using a second cache for path based metrics before.

* Removing the glyph to bounds map saves memory because of removing
  redundant caching between Blink and Skia. Anecdotal: Measuring using
  https://codereview.chromium.org/2123193002 an clicking through a few
  links in the English wikipedia in single process mode leads to the
  glyphToBoundsMap growing to a size of 1.2MB - removing this is a big
  improvement for the Android WebView, which runs in single process mode
  and accumulates space in this cache.

BUG=610313, 589462
R=kojii,tzik
==========

[commit-bot: I haz the power, 2016-07-21 15:58:07]

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

[commit-bot: I haz the power, 2016-07-21 15:58:08]

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

[eae, 2016-07-21 17:56:02]

This is great! One question however.

LGTM w/nit

https://codereview.chromium.org/1980913002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/skia/SkiaTextMetrics.cpp (right):

https://codereview.chromium.org/1980913002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/skia/SkiaTextMetrics.cpp:35: if (width)
{
It looks like we call this with either a width or an extents argument but never
both. If that is correct it might make more sense to split it into a
getGlypWidth and a getGlyphExtents method to avoid the extra branching.

If it is called with both, however, then keep it as is.

[drott, 2016-07-21 17:58:58]

Description was changed from

==========
Unify glyph metrics to use the same Skia API and remove glyphtoBoundsMap

Use identical glyph metrics based bounds in HarfBuzz callbacks as well
as ShapeResult's glyph bounds calculation, except on Mac, due to Skia
bug https://bugs.chromium.org/p/skia/issues/detail?id=5328

This CL should be a win in memory and performance on platforms except
Mac, especially on Android WebView:

* Using the same glyph bounds callback in HarfBuzz shaping and in
  ShapeResult's glyph bounds calculation enables reusing of the same
  cache in Skia, as opposed to using the cached glyph metrics based
  cache vs. using a second cache for path based metrics before.

* Removing the glyph to bounds map saves memory because of removing
  redundant caching between Blink and Skia. Anecdotal: Measuring using
  https://codereview.chromium.org/2123193002 an clicking through a few
  links in the English wikipedia in single process mode leads to the
  glyphToBoundsMap growing to a size of 1.2MB - removing this is a big
  improvement for the Android WebView, which runs in single process mode
  and accumulates space in this cache.

BUG=610313, 589462
R=kojii,tzik
==========

to

==========
Unify glyph metrics to use the same Skia API and remove glyphtoBoundsMap

Use identical glyph metrics based bounds in HarfBuzz callbacks as well
as ShapeResult's glyph bounds calculation, except on Mac, due to Skia
bug https://bugs.chromium.org/p/skia/issues/detail?id=5328

This CL should be a win in memory and performance on platforms except
Mac, especially on Android WebView:

* Using the same glyph bounds callback in HarfBuzz shaping and in
  ShapeResult's glyph bounds calculation enables reusing of the same
  cache in Skia, as opposed to using the cached glyph metrics based
  cache vs. using a second cache for path based metrics before.

* Removing the glyph to bounds map saves memory because of removing
  redundant caching between Blink and Skia. Anecdotal: Measuring using
  https://codereview.chromium.org/2123193002 and clicking through a few
  links in the English wikipedia in single process mode leads to the
  glyphToBoundsMap growing to a size of 1.2MB - removing this is a big
  improvement for the Android WebView, which runs in single process mode
  and accumulates space in this cache.

BUG=610313, 589462
R=kojii,tzik
==========

[drott, 2016-07-21 18:00:24]

Thanks for the review.

https://codereview.chromium.org/1980913002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/skia/SkiaTextMetrics.cpp (right):

https://codereview.chromium.org/1980913002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/skia/SkiaTextMetrics.cpp:35: if (width)
{
On 2016/07/21 at 17:56:02, eae wrote:
> It looks like we call this with either a width or an extents argument but
never both. If that is correct it might make more sense to split it into a
getGlypWidth and a getGlyphExtents method to avoid the extra branching.
> 
> If it is called with both, however, then keep it as is.

Agree, we can avoid the extra branch - with some code redundancy perhaps. Since
I moved this code from HarfBuzzFace and did not change this part, I'd like to do
the change you suggest in a separate CL, if that's okay.

[drott, 2016-07-21 18:02:45]

The CQ bit was checked by drott@chromium.org

[drott, 2016-07-21 18:02:45]

The patchset sent to the CQ was uploaded after l-g-t-m from tzik@chromium.org
Link to the patchset: https://codereview.chromium.org/1980913002/#ps200001
(title: "Rebaseline more win failures")

[commit-bot: I haz the power, 2016-07-21 18:03:25]

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-07-21 20:24:53]

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

[commit-bot: I haz the power, 2016-07-21 20:24:55]

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

[drott, 2016-07-22 07:19:08]

drott@chromium.org changed reviewers:
+ fmalita@chromium.org
- wkorman@chromium.org

[drott, 2016-07-22 14:30:18]

Not landing this yet. I am investigating a performance issue on the failing mac
test:
editing/selection/modify_move/move-by-word-visually-crash-test-5.html
which is already marked as slow. This test seems to exercise the Range API very
heavily and the time for this test is going up from somewhere around 5 seconds
to around 114 seconds.

[drott, 2016-07-25 14:19:04]

Description was changed from

==========
Unify glyph metrics to use the same Skia API and remove glyphtoBoundsMap

Use identical glyph metrics based bounds in HarfBuzz callbacks as well
as ShapeResult's glyph bounds calculation, except on Mac, due to Skia
bug https://bugs.chromium.org/p/skia/issues/detail?id=5328

This CL should be a win in memory and performance on platforms except
Mac, especially on Android WebView:

* Using the same glyph bounds callback in HarfBuzz shaping and in
  ShapeResult's glyph bounds calculation enables reusing of the same
  cache in Skia, as opposed to using the cached glyph metrics based
  cache vs. using a second cache for path based metrics before.

* Removing the glyph to bounds map saves memory because of removing
  redundant caching between Blink and Skia. Anecdotal: Measuring using
  https://codereview.chromium.org/2123193002 and clicking through a few
  links in the English wikipedia in single process mode leads to the
  glyphToBoundsMap growing to a size of 1.2MB - removing this is a big
  improvement for the Android WebView, which runs in single process mode
  and accumulates space in this cache.

BUG=610313, 589462
R=kojii,tzik
==========

to

==========
Unify glyph metrics to use the same Skia API

Use identical glyph metrics based bounds in HarfBuzz callbacks as well
as ShapeResult's glyph bounds calculation, except on Mac, due to Skia
bug https://bugs.chromium.org/p/skia/issues/detail?id=5328

* Using the same glyph bounds callback in HarfBuzz shaping and in
  ShapeResult's glyph bounds calculation enables reusing of the same
  cache in Skia, as opposed to using the cached glyph metrics based
  cache vs. using a second cache for path based metrics before.

Local benchmarking of the blink_perf.layout shows 12.44%
improvements for the character_fallback test and 1.8-5%
improvements on chapter-reflow-once, line-layout and
latin-complex-test. There are also some hits on flexbox-lots of
data and flexbox-row-nowrap. I am not 100% convinced that the
local benchmarks are accurate enough and would like to observe
the results on the bot. Overall, we should see a layout speed
improvement, perhaps even in the page cyclers.

BUG=610313
R=kojii,tzik
==========

[drott, 2016-07-25 14:19:22]

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

[commit-bot: I haz the power, 2016-07-25 14:19:35]

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

[drott, 2016-07-25 14:21:36]

Updated the CL description and not removing glyph to bounds map yet. I don't
fully understand why the bounds and width functions are so hot that they still
require the inlining and aggressive caching. I'll file a bug about that and
investigate a bit more. But when I remove the glyphToBounds map our Range API
and selection code becomes two orders of magnitude slower on Mac and
editing/selection/modify_move/resources/move-by-word-visually.js fails as timing
out.

May I ask for another review? I'd like to land this without the glyphToBoundsMap
removal as it still improves the cache hits below Blink and uses the correct
metrics for text.

[commit-bot: I haz the power, 2016-07-25 14:29:47]

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

[commit-bot: I haz the power, 2016-07-25 14:29:49]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kojii, 2016-07-25 16:36:28]

That's really a great finding for editing/selection to improve.

https://codereview.chromium.org/1980913002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/blink_platform.gypi (left):

https://codereview.chromium.org/1980913002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/blink_platform.gypi:421:
'fonts/GlyphMetricsMap.h',
Shouldn't we need this then?

https://codereview.chromium.org/1980913002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/SimpleFontData.h (right):

https://codereview.chromium.org/1980913002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/SimpleFontData.h:214:
SkASSERT(sizeof(glyph) == 2); // compile-time assert
Our current style guide allows the use of static_assert().

[drott, 2016-07-26 07:48:37]

Addressed Koji's review comments

[drott, 2016-07-26 07:48:39]

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

[commit-bot: I haz the power, 2016-07-26 07:49:00]

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

[drott, 2016-07-26 07:49:36]

Thanks for the review, updated.

https://codereview.chromium.org/1980913002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/blink_platform.gypi (left):

https://codereview.chromium.org/1980913002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/blink_platform.gypi:421:
'fonts/GlyphMetricsMap.h',
On 2016/07/25 at 16:36:28, kojii wrote:
> Shouldn't we need this then?

Absolutely. Added back in.

https://codereview.chromium.org/1980913002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/SimpleFontData.h (right):

https://codereview.chromium.org/1980913002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/SimpleFontData.h:214:
SkASSERT(sizeof(glyph) == 2); // compile-time assert
On 2016/07/25 at 16:36:28, kojii wrote:
> Our current style guide allows the use of static_assert().

Thanks, updated.

[commit-bot: I haz the power, 2016-07-26 07:58:07]

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

[commit-bot: I haz the power, 2016-07-26 07:58:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[drott, 2016-07-26 08:02:31]

Removed a spurious header line

[drott, 2016-07-26 08:02:32]

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

[commit-bot: I haz the power, 2016-07-26 08:02:53]

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

[kojii, 2016-07-26 09:07:32]

lgtm

[commit-bot: I haz the power, 2016-07-26 09:16:20]

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

[commit-bot: I haz the power, 2016-07-26 09:16:21]

Dry run: This issue passed the CQ dry run.

[drott, 2016-07-26 09:54:12]

The CQ bit was checked by drott@chromium.org

[drott, 2016-07-26 09:54:13]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org,
tzik@chromium.org
Link to the patchset: https://codereview.chromium.org/1980913002/#ps260001
(title: "Removed a spurious header line")

[commit-bot: I haz the power, 2016-07-26 09:54:25]

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-07-26 09:57:56]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-07-26 09:59:44]

Description was changed from

==========
Unify glyph metrics to use the same Skia API

Use identical glyph metrics based bounds in HarfBuzz callbacks as well
as ShapeResult's glyph bounds calculation, except on Mac, due to Skia
bug https://bugs.chromium.org/p/skia/issues/detail?id=5328

* Using the same glyph bounds callback in HarfBuzz shaping and in
  ShapeResult's glyph bounds calculation enables reusing of the same
  cache in Skia, as opposed to using the cached glyph metrics based
  cache vs. using a second cache for path based metrics before.

Local benchmarking of the blink_perf.layout shows 12.44%
improvements for the character_fallback test and 1.8-5%
improvements on chapter-reflow-once, line-layout and
latin-complex-test. There are also some hits on flexbox-lots of
data and flexbox-row-nowrap. I am not 100% convinced that the
local benchmarks are accurate enough and would like to observe
the results on the bot. Overall, we should see a layout speed
improvement, perhaps even in the page cyclers.

BUG=610313
R=kojii,tzik
==========

to

==========
Unify glyph metrics to use the same Skia API

Use identical glyph metrics based bounds in HarfBuzz callbacks as well
as ShapeResult's glyph bounds calculation, except on Mac, due to Skia
bug https://bugs.chromium.org/p/skia/issues/detail?id=5328

* Using the same glyph bounds callback in HarfBuzz shaping and in
  ShapeResult's glyph bounds calculation enables reusing of the same
  cache in Skia, as opposed to using the cached glyph metrics based
  cache vs. using a second cache for path based metrics before.

Local benchmarking of the blink_perf.layout shows 12.44%
improvements for the character_fallback test and 1.8-5%
improvements on chapter-reflow-once, line-layout and
latin-complex-test. There are also some hits on flexbox-lots of
data and flexbox-row-nowrap. I am not 100% convinced that the
local benchmarks are accurate enough and would like to observe
the results on the bot. Overall, we should see a layout speed
improvement, perhaps even in the page cyclers.

BUG=610313
R=kojii,tzik

Committed: https://crrev.com/9e47ba024f0061ea667565451c2842f66813fee7
Cr-Commit-Position: refs/heads/master@{#407755}
==========

[commit-bot: I haz the power, 2016-07-26 09:59:46]

Patchset 14 (id:??) landed as
https://crrev.com/9e47ba024f0061ea667565451c2842f66813fee7
Cr-Commit-Position: refs/heads/master@{#407755}