Move glyph lookup to hb-ot-font and remove glyph cache in HarfBuzzFace

Move glyph lookup to hb-ot-font and remove glyph cache in HarfBuzzFace

Together with Behdad we identified several ways to optimize and cleanup
HarfBuzzFace:
* We moved the glyph lookup completely to the built-in HarfBuzz
  implementation in hb-ot-font.
* This allows us to remove the unbounded glyph cache we kept around in
  HarfBuzzFace.
* We also moved instantantion and ownership of HarfBuzzFontData to
  HarfBuzzFace. We only update the size and unicode-range on each call
  to getScaledFont(), saving us as the allocation and setup costs of
  this object for each shaping call.
* We removed the unneeded callback for retrieving the horizontal origin,
  which was implemented as a noop.

In local measurements on Mac this leads to large improvements in the
blink_perf.layout tests, especially in
chapter-reflow-once-random (>130%), flexbox-lots-of-data (>58%) and
ArabicLineLayout (>26%) with only a few of the microbenchmarks being
slightly slower: So this is an improvement for layout cases that do not
profit much from the word cache.

Note to ChromeOS developers: If this change breaks your workflow linking
release builds with system HarfBuzz because of an old version of
HarfBuzz on the host system, we recommend to install and apt-pin the
same version of the HarfBuzz library on your host system that ChromeOS
uses. Please see bug 589342.

BUG=589340
R=eae,behdad,kojii

Committed: https://crrev.com/85a2c1c0597a31b043e58fe4a4ce00a88ebc0684
Cr-Commit-Position: refs/heads/master@{#377849}

[kojii, 2016-02-25 07:55:17]

I understand much better now, thanks for doing this. So we used to create a
SimpleFontData and an hb_font_t on every shaping...that's a great saving.

I would split out the change that is likely to break cros-on-linux into a
separate patch to avoid this whole good thing being reverted while we're
sleeping and try to re-land etc., but it's up to you.

One more question from curiosity...are we trying to land before the branch, or
after?

https://codereview.chromium.org/1733193002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp (right):

https://codereview.chromium.org/1733193002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp:296:
HarfBuzzScopedPtr<hb_font_t> otFont(hb_font_create(m_face), hb_font_destroy);
HarfBuzzScopedPtr is not needed for hb_font_t as I have added
OwnedPtrDeleter<hb_font_t> before in:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

so just |OwnPtr<hb_font_t>| should work, like this:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[kojii, 2016-02-25 08:11:34]

On 2016/02/25 at 07:55:17, kojii wrote:
> I would split out...

ah, never mind this. I noticed the new code also require up-to-date hb, so
leaving existing #if doesn't really help.

[drott, 2016-02-25 09:03:51]

Use OwnPtr instead of ScopedPtr

[drott, 2016-02-25 09:06:00]

https://codereview.chromium.org/1733193002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp (right):

https://codereview.chromium.org/1733193002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp:296:
HarfBuzzScopedPtr<hb_font_t> otFont(hb_font_create(m_face), hb_font_destroy);
Thanks, good point, updated accordingly.

[drott, 2016-02-25 09:08:43]

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

[commit-bot: I haz the power, 2016-02-25 09:09:33]

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

[commit-bot: I haz the power, 2016-02-25 10:12:07]

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

[commit-bot: I haz the power, 2016-02-25 10:12:08]

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

[eae, 2016-02-25 18:02:38]

Very nice!

LGTM

[drott, 2016-02-26 00:47:08]

The CQ bit was checked by drott@chromium.org

[commit-bot: I haz the power, 2016-02-26 00:54:28]

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

[commit-bot: I haz the power, 2016-02-26 02:42:38]

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

[commit-bot: I haz the power, 2016-02-26 02:42:39]

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

[drott, 2016-02-26 03:48:06]

updated memory management, no more ref'ing, OwnPtr in HarfBuzzFace

[drott, 2016-02-26 03:48:45]

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

[commit-bot: I haz the power, 2016-02-26 03:49:58]

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

[commit-bot: I haz the power, 2016-02-26 05:04:48]

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

[commit-bot: I haz the power, 2016-02-26 05:04:49]

Dry run: This issue passed the CQ dry run.

[kojii, 2016-02-26 06:57:57]

lgtm!!

[drott, 2016-02-26 07:50:33]

The CQ bit was checked by drott@chromium.org

[drott, 2016-02-26 07:50:33]

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/1733193002/#ps40001
(title: "updated memory management, no more ref'ing, OwnPtr in HarfBuzzFace")

[commit-bot: I haz the power, 2016-02-26 07:51:04]

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

[commit-bot: I haz the power, 2016-02-26 08:16:05]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-26 08:18:34]

Description was changed from

==========
Move glyph lookup to hb-ot-font and remove glyph cache in HarfBuzzFace

Together with Behdad we identified several ways to optimize and cleanup
HarfBuzzFace:
* We moved the glyph lookup completely to the built-in HarfBuzz
  implementation in hb-ot-font.
* This allows us to remove the unbounded glyph cache we kept around in
  HarfBuzzFace.
* We also moved instantantion and ownership of HarfBuzzFontData to
  HarfBuzzFace. We only update the size and unicode-range on each call
  to getScaledFont(), saving us as the allocation and setup costs of
  this object for each shaping call.
* We removed the unneeded callback for retrieving the horizontal origin,
  which was implemented as a noop.

In local measurements on Mac this leads to large improvements in the
blink_perf.layout tests, especially in
chapter-reflow-once-random (>130%), flexbox-lots-of-data (>58%) and
ArabicLineLayout (>26%) with only a few of the microbenchmarks being
slightly slower: So this is an improvement for layout cases that do not
profit much from the word cache.

Note to ChromeOS developers: If this change breaks your workflow linking
release builds with system HarfBuzz because of an old version of
HarfBuzz on the host system, we recommend to install and apt-pin the
same version of the HarfBuzz library on your host system that ChromeOS
uses. Please see bug 589342.

BUG=589340
R=eae,behdad,kojii
==========

to

==========
Move glyph lookup to hb-ot-font and remove glyph cache in HarfBuzzFace

Together with Behdad we identified several ways to optimize and cleanup
HarfBuzzFace:
* We moved the glyph lookup completely to the built-in HarfBuzz
  implementation in hb-ot-font.
* This allows us to remove the unbounded glyph cache we kept around in
  HarfBuzzFace.
* We also moved instantantion and ownership of HarfBuzzFontData to
  HarfBuzzFace. We only update the size and unicode-range on each call
  to getScaledFont(), saving us as the allocation and setup costs of
  this object for each shaping call.
* We removed the unneeded callback for retrieving the horizontal origin,
  which was implemented as a noop.

In local measurements on Mac this leads to large improvements in the
blink_perf.layout tests, especially in
chapter-reflow-once-random (>130%), flexbox-lots-of-data (>58%) and
ArabicLineLayout (>26%) with only a few of the microbenchmarks being
slightly slower: So this is an improvement for layout cases that do not
profit much from the word cache.

Note to ChromeOS developers: If this change breaks your workflow linking
release builds with system HarfBuzz because of an old version of
HarfBuzz on the host system, we recommend to install and apt-pin the
same version of the HarfBuzz library on your host system that ChromeOS
uses. Please see bug 589342.

BUG=589340
R=eae,behdad,kojii

Committed: https://crrev.com/85a2c1c0597a31b043e58fe4a4ce00a88ebc0684
Cr-Commit-Position: refs/heads/master@{#377849}
==========

[commit-bot: I haz the power, 2016-02-26 08:18:35]

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

[achuithb, 2016-02-26 10:29:53]

This breaks my compile:

../../third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp:296:5:
error: use of undeclared identifier 'hb_ot_font_set_funcs'
    hb_ot_font_set_funcs(otFont.get());
    ^
1 error generated.

[drott, 2016-02-26 11:12:23]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1742593003/ by drott@chromium.org.

The reason for reverting is: It seems to break chromeos=1, not only buildtype
official. Reverting and will start discussing an agreeable solution on
chrome-dev..