Symbolized output from a sample.

I'm not surprised it's stuck in +[MenuController elideMenuTitle:toWidth:]

What is surprising is that BookmarkMenuBridge::UpdateMenuInternal() is being called every time the menu is shown. BookmarkMenuBridge is meant to cache its model, and only invalidate it when a bookmark changes.

Maybe sync is going berserk and sending me updates to my bookmarks, or something has regressed around this..


    + !                                                                                   1684 Check1MenuForKeyEvent(MenuData*, CheckMenuData*)  (in HIToolbox) + 2256  [0x7fff7ab42124]
    + !                                                                                     1684 Check1MenuForKeyEvent(MenuData*, CheckMenuData*)  (in HIToolbox) + 278  [0x7fff7ab4196a]
    + !                                                                                       1684 PopulateMenu(MenuData*, OpaqueEventTargetRef*, CheckMenuData*, unsigned int, double)  (in HIToolbox) + 90  [0x7fff7ab42246]
    + !                                                                                         1684 SendMenuPopulate(MenuData*, OpaqueEventTargetRef*, unsigned int, double, unsigned int, OpaqueEventRef*, unsigned char, unsigned char*)  (in HIToolbox) + 320  [0x7fff7ab424a9]
    + !                                                                                           1684 SendEventToEventTargetWithOptions  (in HIToolbox) + 43  [0x7fff7aaf9e3f]
    + !                                                                                             1684 SendEventToEventTargetInternal(OpaqueEventRef*, OpaqueEventTargetRef*, HandlerCallRec*)  (in HIToolbox) + 428  [0x7fff7aaf9ff6]
    + !                                                                                               1684 DispatchEventToHandlers(EventTargetRec*, OpaqueEventRef*, HandlerCallRec*)  (in HIToolbox) + 1708  [0x7fff7aafad85]
    + !                                                                                                 1684 NSSLMMenuEventHandler  (in AppKit) + 1093  [0x7fff7924a5eb]
    + !                                                                                                   1684 -[NSCarbonMenuImpl _carbonPopulateEvent:handlerCallRef:]  (in AppKit) + 468  [0x7fff7924a8b5]
    + !                                                                                                     1684 -[NSMenu _populateWithEventRef:]  (in AppKit) + 84  [0x7fff79248079]
    + !                                                                                                       1684 -[NSMenu _populateFromDelegateWithEventRef:]  (in AppKit) + 334  [0x7fff7924b75e]
    + !                                                                                                         1684 BookmarkMenuBridge::UpdateMenuInternal(NSMenu*, bool)  (in Google Chrome Framework)  load address 0x108381000 + 0x43eddcd  [bookmark_menu_bridge.mm:0]
    + !                                                                                                           1682 BookmarkMenuBridge::AddNodeToMenu(bookmarks::BookmarkNode const*, NSMenu*, bool)  (in Google Chrome Framework)  load address 0x108381000 + 0x43ee4d1  [bookmark_menu_bridge.mm:272]
    + !                                                                                                           : 1006 BookmarkMenuBridge::AddNodeToMenu(bookmarks::BookmarkNode const*, NSMenu*, bool)  (in Google Chrome Framework)  load address 0x108381000 + 0x43ee33d  [bookmark_menu_bridge.mm:261]
    + !                                                                                                           : | 1006 +[BookmarkMenuCocoaController menuTitleForNode:]  (in Google Chrome Framework)  load address 0x108381000 + 0x43ee999  [bookmark_menu_cocoa_controller.mm:41]
    + !                                                                                                           : |   1004 +[MenuController elideMenuTitle:toWidth:]  (in Google Chrome Framework)  load address 0x108381000 + 0x2e25d06  [menu_controller.mm:0]
    + !                                                                                                           : |   + 1003 gfx::ElideText(std::__1::basic_string<unsigned short, base::string16_internals::string16_char_traits, std::__1::allocator<unsigned short> > const&, gfx::FontList const&, float, gfx::ElideBehavior)  (in Google Chrome Framework)  load address 0x108381000 + 0x27d3ee6  [text_elider.cc:213]
    + !                                                                                                           : |   + ! 1003 gfx::RenderTextHarfBuzz::GetDisplayText()  (in Google Chrome Framework)  load address 0x108381000 + 0x27dc6ba  [render_text.h:509]
    + !                                                                                                           : |   + !   604 gfx::RenderTextHarfBuzz::EnsureLayoutRunList()  (in Google Chrome Framework)  load address 0x108381000 + 0x27dc959  [render_text.h:509]
    + !                                                                                                           : |   + !   : 604 gfx::RenderText::UpdateDisplayText(float)  (in Google Chrome Framework)  load address 0x108381000 + 0x27ea3cc  [string:1221]

again, with less wrapping. and the actual attachment that crbug ate when my change collided with another.

Deleted: symbolized.txt 1.4 MB

symbolized.txt 1.4 MB View Download

1684 Check1MenuForKeyEvent(MenuData*, CheckMenuData*)  (in HIToolbox) + 2256  [0x7fff7ab42124]
  1684 Check1MenuForKeyEvent(MenuData*, CheckMenuData*)  (in HIToolbox) + 278  [0x7fff7ab4196a]
    1684 PopulateMenu(MenuData*, OpaqueEventTargetRef*, CheckMenuData*, unsigned int, double)  (in HIToolbox) + 90  [0x7fff7ab42246]
      1684 SendMenuPopulate(MenuData*, OpaqueEventTargetRef*, unsigned int, double, unsigned int, OpaqueEventRef*, unsigned char, unsigned char*)  (in HIToolbox) + 320  [0x7fff7ab424a9]
        1684 SendEventToEventTargetWithOptions  (in HIToolbox) + 43  [0x7fff7aaf9e3f]
          1684 SendEventToEventTargetInternal(OpaqueEventRef*, OpaqueEventTargetRef*, HandlerCallRec*)  (in HIToolbox) + 428  [0x7fff7aaf9ff6]
            1684 DispatchEventToHandlers(EventTargetRec*, OpaqueEventRef*, HandlerCallRec*)  (in HIToolbox) + 1708  [0x7fff7aafad85]
              1684 NSSLMMenuEventHandler  (in AppKit) + 1093  [0x7fff7924a5eb]
                1684 -[NSCarbonMenuImpl _carbonPopulateEvent:handlerCallRef:]  (in AppKit) + 468  [0x7fff7924a8b5]
                  1684 -[NSMenu _populateWithEventRef:]  (in AppKit) + 84  [0x7fff79248079]
                    1684 -[NSMenu _populateFromDelegateWithEventRef:]  (in AppKit) + 334  [0x7fff7924b75e]
                      1684 BookmarkMenuBridge::UpdateMenuInternal(NSMenu*, bool)  (in Google Chrome Framework)  load address 0x108381000 + 0x43eddcd  [bookmark_menu_bridge.mm:0]
                        1682 BookmarkMenuBridge::AddNodeToMenu(bookmarks::BookmarkNode const*, NSMenu*, bool)  (in Google Chrome Framework)  load address 0x108381000 + 0x43ee4d1  [bookmark_menu_bridge.mm:272]
                        : 1006 BookmarkMenuBridge::AddNodeToMenu(bookmarks::BookmarkNode const*, NSMenu*, bool)  (in Google Chrome Framework)  load address 0x108381000 + 0x43ee33d  [bookmark_menu_bridge.mm:261]
                        : | 1006 +[BookmarkMenuCocoaController menuTitleForNode:]  (in Google Chrome Framework)  load address 0x108381000 + 0x43ee999  [bookmark_menu_cocoa_controller.mm:41]
                        : |   1004 +[MenuController elideMenuTitle:toWidth:]  (in Google Chrome Framework)  load address 0x108381000 + 0x2e25d06  [menu_controller.mm:0]
                        : |   + 1003 gfx::ElideText(std::__1::basic_string<unsigned short, base::string16_internals::string16_char_traits, std::__1::allocator<unsigned short> > const&, gfx::FontList const&, float, gfx::ElideBehavior)  (in Google Chrome Framework)  load address 0x108381000 + 0x27d3ee6  [text_elider.cc:213]
                        : |    address 0x108381000 + 0x27dc6ba  [render_text.h:509]
                        : |     load address 0x108381000 + 0x27dc959  [render_text.h:509]
                        : |   oad address 0x108381000 + 0x27ea3cc  [string:1221]

I bisected this down to r513541 -> https://chromium.googlesource.com/chromium/src/+/d9338280305c81f3dac8aa1a3da3e459e3f341f9

  Roll HarfBuzz to 1.6.3

  Contains required changes to fix trak table to support on Mac (issue
  627609), and Unicode 10 updates, an ICU build fix and fixes for
  hb_set.


So this probably also needs chrome://flags/#enable-harfbuzz-rendertext enabled

It looks like there's a massive performance hit on Mac with that HarfBuzz roll.

The hotspot seems to be hb_coretext_shaper_font_data_ensure -- looks like it calls CTFontCreateUIFontForLanguage() on every call rather than relying on the CGFont wrapped up in hb_face_t

For UI, the wrapping occurs in harfbuzz_font_skia.cc with a call to hb_coretext_face_create(cg_font) to get the hb_face_t -- harfbuzz shouldn't need to call CTFontCreateUIFontForLanguage()

maybe hb_coretext_face_create should take a CTFontRef instead of a CGFontRef? -- we currently call CTFontCopyGraphicsFont() on a CTFont to make it CGFont in order to call that method. But then _hb_coretext_shaper_font_data_create invokes create_ct_font on the CGFont to get a CTFont again.

Behdad, is this an issue with the way RenderTextHarfBuzz uses HarfBuzz? Is the hb_font recreated too often? Or did we miss something in the CoreText tracking improvements?

CC'ed ellyjones@ and karandeepb@ - do you know more about the HarfBuzz usage pattern when the menu is opened?

Don't really have much context here.

I don't think there's anything special about the menu use case -- it's just doing a text layout, which is now very slow when using a hb_face_t returned by hb_coretext_face_create(). The same hb_face_t is being used each time, but hb_coretext_shaper_font_data_ensure isn't keeping its work.  There's probably a big performance hit to use-cases in the renderer and elsewhere as well.

I think the changes in the HarfBuzz roll were to fix  Issue 627609  in the renderer.

On the UI side, however, we typically start with a CoreText handle already. Both Chrome [1] and Skia [2] cache it.

But now harfbuzz is taking a CoreGraphics font handle and re-determining the CoreText flavour on each shape pass, defeating any caching being done at a higher level.

I think we need to store the CTFontRef in the hb_face_t -- not the CGFontRef -- and update hb_coretext_shaper_font_data_ensure to use it. That will help things on the UI side -- it's easy to pass a CTFontRef to hb_coretext_face_create.


But I'm not sure what the right fix is on the renderer side. I think at a minimum, the stuff being done in hb_coretext_shaper_font_data_ensure can be moved to hb_coretext_face_create. That will resolve cases where client code really only wants to pass a CoreGraphics handle. However, a better fix may be to have the renderer track CoreText handles instead, and pass the result of CTFontCopyGraphicsFont() to the harfbuzz layer explicitly.


[1] https://cs.chromium.org/chromium/src/ui/gfx/harfbuzz_font_skia.cc?q=harfbuzz_font_skia.cc&sq=package:chromium&l=289

[2] https://cs.chromium.org/chromium/src/third_party/skia/src/ports/SkFontHost_mac.cpp?type=cs&q=create_from_CTFontRef&l=548

The change we made causes recreation of CTFont every time font size changes on hb_font_t.  There's not much we can do to avoid that while still keeping optical sizing working.  This smells, I'm guessing that the eliding code is doing something phishy.  First thing to check is how often hb_font_t's are created.  They used to be really cheap.  Now they also create CTFont, so not as cheap as they used to be.

I added some tracing and tried including the font size in the key to hb_face_t
 -- https://chromium-review.googlesource.com/c/chromium/src/+/776337

it didn't have any effect.

hb_shape seems to call CTFontCreateUIFontForLanguage unconditionally.

The tracing results in a spew of

face = 0x7fbdb47d3d90, cg_font = 0x7fbdb6e45230create for size 36.000000
face = 0x7fbdb47d3d90, cg_font = 0x7fbdb6e45230create for size 36.000000
face = 0x7fbdb47d3d90, cg_font = 0x7fbdb6e45230create for size 36.000000
face = 0x7fbdb47da1d0, cg_font = 0x7fbdb6e45230create for size 36.000000
face = 0x7fbdb47da1d0, cg_font = 0x7fbdb6e45230create for size 36.000000
face = 0x7fbdb47da1d0, cg_font = 0x7fbdb6e45230create for size 36.000000
face = 0x7fbdb47da1d0, cg_font = 0x7fbdb6e45230create for size 36.000000
face = 0x7fbdb47da1d0, cg_font = 0x7fbdb6e45230create for size 36.000000
face = 0x7fbdb47da1d0, cg_font = 0x7fbdb6e45230create for size 36.000000
face = 0x7fbdb47da1d0, cg_font = 0x7fbdb6e45230create for size 36.000000
face = 0x7fbdb47da1d0, cg_font = 0x7fbdb6e45230create for size 36.000000
face = 0x7fbdb47da1d0, cg_font = 0x7fbdb6e45230create for size 36.000000
face = 0x7fbdb47da1d0, cg_font = 0x7fbdb6e45230create for size 36.000000
face = 0x7fbdb47da1d0, cg_font = 0x7fbdb6e45230create for size 36.000000
face = 0x7fbdb47da740, cg_font = 0x7fbdb6e45230create for size 36.000000
face = 0x7fbdb47da740, cg_font = 0x7fbdb6e45230create for size 36.000000
face = 0x7fbdb47d83d0, cg_font = 0x7fbdb6e45230create for size 36.000000
face = 0x7fbdb47d83d0, cg_font = 0x7fbdb6e45230create for size 36.000000
face = 0x7fbdb47d83d0, cg_font = 0x7fbdb6e45230create for size 36.000000
face = 0x7fbdb47d83d0, cg_font = 0x7fbdb6e45230create for size 36.000000
face = 0x7fbdb47d83d0, cg_font = 0x7fbdb6e45230create for size 36.000000

Right. So the way gfx::RenderText is written for all platforms is to cache a hb_*face*_t. hb_*font*_t is recreated for every text run at https://cs.chromium.org/RenderTextHarfBuzz::ShapeRunWithFont()

So it's the performance of hb_font_t construction that has regressed significantly.

I don't see an easy way to cache that :/

Is there any reason why hb_coretext_face_create() can't just take a CTFontRef?  (or can we set the CTFontRef on an hb_face_t* afterward, explicitly?)

Blink is doing the same thing as the UI code. It already has a CTFontRef, and has to convert it to CGFontRef to pass it to harfbuzz:

CGFontRef FontPlatformData::CgFont() const {
  if (!CtFont())
    return nullptr;
  return CTFontCopyGraphicsFont(CtFont(), 0);
}

Yes, there are reasons for that. In workshops with Behdad we've discussed this extensively and decided it's not the right approach. The main reason is the matching level of abstraction: hb_face is a "sizeless" font handle, hb_font has a size. A CGFont is also sizeless, and thus fits hb_face better. 

Details in these discussions:
https://github.com/behdad/harfbuzz/issues/361
https://github.com/behdad/harfbuzz/pull/367

> So it's the performance of hb_font_t construction that has regressed significantly.

hb_font_t construction is now more costly only for fonts that match ".NSSFText" or ".NSSFDisplay". So it is more costly for Mac system fonts, not in general.

This is because the HarfBuzz roll fixes an important letter spacing / tracking issue and fixes github rendering among other things, compare issue:  627609 , a highly starred issue for rendering on Mac. In fact, this also opens up the possibility to fix tracking for RenderTextHarfBuzz if the hb_set_ptem call is added after hb_font_set_scale here:
https://cs.chromium.org/chromium/src/ui/gfx/harfbuzz_font_skia.cc?sq=package:chromium&l=297


The renderer code cache hb_font_t objects and just "rescales" them, compare:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFontCache.h?dr&sq=package:chromium&l=93
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp?q=getscaledfont&sq=package:chromium&l=335

I believe something similar might be possible on the UI side, instead of having a FaceCache here: https://cs.chromium.org/chromium/src/ui/gfx/harfbuzz_font_skia.cc?sq=package:chromium&l=291
This could be a HbFontCache, and then just rescale on every run-shape.

And https://cs.chromium.org/chromium/src/ui/gfx/harfbuzz_font_skia.cc?sq=package:chromium&l=284 CreateHarfBuzzFont should become something like "ScaleHarfBuzzFont".

How about an interface for just setting the CTFontRef on the hb_font_t explicitly?

Even if we cache the hb_font_t it doesn't seem fair to penalise callers with cost of invoking CTFontCreateUIFontForLanguage() when they already have the correct CTFontRef.

CTFontCreateUIFontForLanguage() is the only way to ensure correct size dependent tracking / letter spacing. In other words: If an ".NSSFDisplay" or ".NSSFText" CTFontRef of different origin was created only by using the font family name, you won't get correct tracking for these fonts. This is - if you will - a CoreText issue, as there is no other known CoreText API that can activate correct tracking on an existing CTFontRef.

Yes, it's precisely for that reason that we don't derive fonts by font family name in UI. Everything is done relative to gfx::Font() which creates a 

PlatformFontMac::PlatformFontMac()
    : PlatformFontMac([NSFont systemFontOfSize:[NSFont systemFontSize]]) {
}

See  Issue 605404  and https://codereview.chromium.org/2222483002

In UI we always have the CTFontRef that we want the typesetter to use.

What's the conclusion here?  If current HarfBuzz performance is good-enough for Blink, I don't see how it's not good-enough for the menus.  Just keep a hb_font_t around.  Is that not feasible?

I *can* cache one CTFont on the hb_face_t *if* we know the font does not use optical sizing.  But that wouldn't solve the problem in this case.

I can add hb_coretext_font_create (CTFontRef), but then higher level would need to change code to forgo hb_face_t.  If that's alright with you, I can take a look.

hb_coretext_font_create(CTFontRef) sounds like what we want.

The core problem is that CTFontCreateUIFontForLanguage() is quite expensive. In UI code, we've already used (and must continue to use) [NSFont systemFontOfSize:], so we already have a CTFont and never want the expense of creating another.

UI text layout is on the critical path of browser startup, so we are very sensitive to this.

See, for example,  Issue 780670  and the charts at https://uma.googleplex.com/p/chrome/callstacks?sid=e2e940591ae082541662b434d930e71a

That issue was on Windows. But after fixing the regression due to base::i18n::BreakIterator being slow, the lion's share of remaining time is spent constructing fonts. That is, 65ms spent in gfx::PlatformFont::CreateDefault and gfx::Font::Derive. (Of course Mac is different, but likely similar wrt font creation).

(About 20ms of the remaining 147ms is spent in hb_shape, and the bulk of the rest we've put down to page faults from memory-mapped files).

> About 20ms of the remaining 147ms is spent in hb_shape

oops that's 147ms *total* in the charts displayed at that link. i.e 44% of time creating fonts (3 fonts, in this case).

Working on new HarfBuzz API here: https://github.com/harfbuzz/harfbuzz/pull/629

791915 is done and contains https://github.com/harfbuzz/harfbuzz/issues/628, new API hb_coretext_font_create(). tapted@, would you perhaps be willing to take a look and incorporate this call? If not, I can try to add it to my list but I am a bit busy until February.

Thanks a ton! I'll give it a go in the next couple of weeks.

Rough plan: a first cut with the existing cache in UI to avoid the font lookup in hb_shape().

Then, I still want to get the UI code caching hb_font_t as well as hb_face_t. Ideally this is done by calling into the Blink code rather than copy-pasting bits of the implementation. That's a big refactor though.

I also want to put in some kind of perf testing. (There's not much infrastructure for doing this on the UI side).

So, a few things to explore.

The first cut improves a basic perf test from 139 runs/second to 306 runs/second and none of the Chrome UI is obviously broken. I'll do a bit more analysis, but it seems promising.

(I think the subscript/superscript codepaths in RenderTextHarfBuzz will be broken, but we don't ship any UI on Mac that actually uses these.... maybe not anywhere.. :/. I'll remove, or disable-on-mac, or otherwise find a nice solution for that.

CL -> https://chromium-review.googlesource.com/823743

Thanks very much for taking this on!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/db7576464e43be800f1439e92550d14ad3b07e7c

commit db7576464e43be800f1439e92550d14ad3b07e7c
Author: Trent Apted <tapted@chromium.org>
Date: Thu Jan 04 04:26:17 2018

Mac RenderTextHarfBuzz: Use hb_coretext_font_create().

The HarfBuzz roll in r513541 improved Mac UI font support in the
renderer by detecting UI fonts and invoking CTFontCreateUIFontForLanguage().

However, in chrome UI, there is already a CTFont wrapped in a gfx::
PlatformFont for UI fonts. So creating a new CTFont for every hb_shape
call becomes unnecessary and expensive.

hb_coretext_font_create() (after a hb_coretext_face_create()) allows
the steps requiring the call to CTFontCreateUIFontForLanguage() to
be skipped by associating the CTFont with the hb_font_t directly.

Adds a new views_perftest target and LabelPerfTest.GetPreferredSize
to track. On a Late 2013 Mac Pro perf improves from 1390 runs/second
to 3300 runs/second.

+250 runs/second of that comes from a related hotspot in gfx::internal::
CreateSkiaTypeface() which was deriving a new CTFont unnecessarily
before sending it to gfx::CreateHarfBuzzFont().

Bug:  785522 
Change-Id: Iccf57fe3fc9a93cd4b9de1e79e81fb84d2ad88bb
Reviewed-on: https://chromium-review.googlesource.com/823743
Commit-Queue: Trent Apted <tapted@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526920}
[modify] https://crrev.com/db7576464e43be800f1439e92550d14ad3b07e7c/ui/gfx/font.cc
[modify] https://crrev.com/db7576464e43be800f1439e92550d14ad3b07e7c/ui/gfx/harfbuzz_font_skia.cc
[modify] https://crrev.com/db7576464e43be800f1439e92550d14ad3b07e7c/ui/views/BUILD.gn
[modify] https://crrev.com/db7576464e43be800f1439e92550d14ad3b07e7c/ui/views/DEPS
[add] https://crrev.com/db7576464e43be800f1439e92550d14ad3b07e7c/ui/views/controls/label_perftest.cc
[add] https://crrev.com/db7576464e43be800f1439e92550d14ad3b07e7c/ui/views/views_perftests.cc

Reverting as this broke
RenderTextHarfBuzzTest.Multiline_ZeroWidthChars/HarfBuzz
RenderTextHarfBuzzTest.HarfBuzz_AsciiVariationSelector/HarfBuzz
on Mac-10.10 and Mac10.12.

Mac-10.11 seems unaffected.

Sample output:
[ RUN      ] RenderTextHarfBuzzTest.Multiline_ZeroWidthChars/HarfBuzz
../../ui/gfx/render_text_unittest.cc:3634: Failure
Expected equality of these values:
  3u
    Which is: 3
  test_api()->lines().size()
    Which is: 4
[  FAILED  ] RenderTextHarfBuzzTest.Multiline_ZeroWidthChars/HarfBuzz, where GetParam() = 0 (2 ms)
https://logs.chromium.org/v/?s=chromium%2Fbb%2Fchromium.mac%2FMac10.10_Tests%2F27713%2F%2B%2Frecipes%2Fsteps%2Fgfx_unittests_on_Mac-10.10%2F0%2Flogs%2FRenderTextHarfBuzzTest.Multiline_ZeroWidthChars__x2f_HarfBuzz%2F0

[ RUN      ] RenderTextHarfBuzzTest.HarfBuzz_AsciiVariationSelector/HarfBuzz
../../ui/gfx/render_text_unittest.cc:577: Failure
Expected: (max_cursor_x) <= (cursor_bounds.x()), actual: 15 vs 14
Google Test trace:
../../ui/gfx/render_text_unittest.cc:571: Cursor position: 3 selection: {3,3}
[  FAILED  ] RenderTextHarfBuzzTest.HarfBuzz_AsciiVariationSelector/HarfBuzz, where GetParam() = 0 (2 ms)
https://logs.chromium.org/v/?s=chromium%2Fbb%2Fchromium.mac%2FMac10.10_Tests%2F27713%2F%2B%2Frecipes%2Fsteps%2Fgfx_unittests_on_Mac-10.10%2F0%2Flogs%2FRenderTextHarfBuzzTest.HarfBuzz_AsciiVariationSelector__x2f_HarfBuzz%2F0

I am going to revert the CL.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e404c97d1356675e58e94722ba9ddb9f37508b4b

commit e404c97d1356675e58e94722ba9ddb9f37508b4b
Author: Dominic Battré <battre@chromium.org>
Date: Thu Jan 04 07:57:38 2018

Revert "Mac RenderTextHarfBuzz: Use hb_coretext_font_create()."

This reverts commit db7576464e43be800f1439e92550d14ad3b07e7c.

Reason for revert: Broke unittests, see bug for details.

Original change's description:
> Mac RenderTextHarfBuzz: Use hb_coretext_font_create().
> 
> The HarfBuzz roll in r513541 improved Mac UI font support in the
> renderer by detecting UI fonts and invoking CTFontCreateUIFontForLanguage().
> 
> However, in chrome UI, there is already a CTFont wrapped in a gfx::
> PlatformFont for UI fonts. So creating a new CTFont for every hb_shape
> call becomes unnecessary and expensive.
> 
> hb_coretext_font_create() (after a hb_coretext_face_create()) allows
> the steps requiring the call to CTFontCreateUIFontForLanguage() to
> be skipped by associating the CTFont with the hb_font_t directly.
> 
> Adds a new views_perftest target and LabelPerfTest.GetPreferredSize
> to track. On a Late 2013 Mac Pro perf improves from 1390 runs/second
> to 3300 runs/second.
> 
> +250 runs/second of that comes from a related hotspot in gfx::internal::
> CreateSkiaTypeface() which was deriving a new CTFont unnecessarily
> before sending it to gfx::CreateHarfBuzzFont().
> 
> Bug:  785522 
> Change-Id: Iccf57fe3fc9a93cd4b9de1e79e81fb84d2ad88bb
> Reviewed-on: https://chromium-review.googlesource.com/823743
> Commit-Queue: Trent Apted <tapted@chromium.org>
> Reviewed-by: Michael Wasserman <msw@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#526920}

TBR=msw@chromium.org,tapted@chromium.org,nednguyen@google.com

Change-Id: I13cbdf3798ba6e3c374e92acfa1932cf9e8d5868
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  785522 
Reviewed-on: https://chromium-review.googlesource.com/848857
Reviewed-by: Dominic Battré <battre@chromium.org>
Commit-Queue: Dominic Battré <battre@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526936}
[modify] https://crrev.com/e404c97d1356675e58e94722ba9ddb9f37508b4b/ui/gfx/font.cc
[modify] https://crrev.com/e404c97d1356675e58e94722ba9ddb9f37508b4b/ui/gfx/harfbuzz_font_skia.cc
[modify] https://crrev.com/e404c97d1356675e58e94722ba9ddb9f37508b4b/ui/views/BUILD.gn
[modify] https://crrev.com/e404c97d1356675e58e94722ba9ddb9f37508b4b/ui/views/DEPS
[delete] https://crrev.com/65e2abcb74b1c07fa14f46abaa1fb1717892eec3/ui/views/controls/label_perftest.cc
[delete] https://crrev.com/65e2abcb74b1c07fa14f46abaa1fb1717892eec3/ui/views/views_perftests.cc

Do we know what broke?
Also, you shouldn't need hb_coretext_face_create() anymore. Right?

Fonts on Mac are forever broke :|. 10.10 is the only macOS that uses 'Helvetica Neue' as the system font. In 10.11, Mac switched to San Francisco but it keeps changing in each release still.

On 10.12.6 and 10.10.5, I get a DCHECK in RenderTextHarfBuzzTest.HarfBuzz_AsciiVariationSelector:

[569:775:0104/231708.729455:549435548137:FATAL:render_text_harfbuzz.cc(752)] Check failed: cluster_begin_x <= cluster_end_x (14.917 vs. 13.9141)
..
5   libgfx.dylib                        0x000000010af7fee5 gfx::internal::TextRunHarfBuzz::GetGraphemeBounds(gfx::RenderTextHarfBuzz*, unsigned long) const + 805
6   libgfx.dylib                        0x000000010af80b72 gfx::internal::TextRunHarfBuzz::GetGraphemeSpanForCharRange(gfx::RenderTextHarfBuzz*, gfx::Range const&) const + 802
7   libgfx.dylib                        0x000000010af84d7c gfx::RenderTextHarfBuzz::GetCursorSpan(gfx::Range const&) + 732
8   libgfx.dylib                        0x000000010afbc9ac gfx::RenderText::GetCursorBounds(gfx::SelectionModel const&, bool) + 1228
9   gfx_unittests                       0x000000010a4ebf80 gfx::RenderTextTest::CheckBoundsForCursorPositions() + 2640
..

The test uses the string "z\uFE0Fy" or "z️y" (which may render as just "zy"..). Mac's font system is interpreting z\uFE0F as "A<gunk>" in all applications. This is low-priority to fix/workaround.

Using Arial rather than a system font makes the test pass -- system font behaviour isn't important for this test. This fixes 10.10 as well.

The failed DCHECK just means the cursor/selection will jump around weirdly in release [tested with the DHCECK commented out].

I can repro the output in #c30 on 10.10 by commenting out the DCHECK and NOT changing to Arial. So changing to Arial fixes both. That test failure is consistent with the "cursor jumps around weirdly" since that's exactly what CheckBoundsForCursorPositions() is checking for.


RenderTextHarfBuzzTest.Multiline_ZeroWidthChars/HarfBuzz only fails on my 10.10 VM. It's testing a ZeroWidthSpace ("\u200B") and expects the string "test\u200B\n\u200Btest.", or

"""
test​
​test.
"""
 to wrap to 3 lines:

test\u200B
\u200Btest
.

On 10.10 it wraps to 4 lines:

test\u200B
\u200B
test
.

test​​test.

It does appear as though, on 10.10, Harfbuzz is giving zero-width spaces a tiny amount of space. Pasting these "​​​​​​​​​​​​​​​​​​​​" 20 zero-width spaces into a views::Textfield on 10.10 results in tiny amount of space (about one "real" space worth). This might be a Harfbuzz bug, since I couldn't repro in Safari.

Deleted: zy-chrome-cocoa.png 10.4 KB

	zy-chrome-cocoa.png 10.4 KB View Download

Deleted: zy-harfbuzz.png 4.3 KB

	zy-harfbuzz.png 4.3 KB View Download

Deleted: zy-safari.png 75.9 KB

	zy-safari.png 75.9 KB View Download

The way this seems to manifest in Blink (for me on 10.10 as well as 10.12) is if you have the string

'x\u200B'

(e.g. go to textarea.org, and paste in "x​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​" (there are bunch of zero width spaces after the "x"))

then place the cursor between the "x" and the zero-width space and press shift+right, then the "x" gets highlighted. Changing the font to Arial causes no highlight.

Filed Issue 799333 for the zero-width space thing.
The ascii-variation-selector thing needs a rdar://, but it's pretty low priority since variation selectors on ascii aren't really a thing.

reland with workarounds is in https://chromium-review.googlesource.com/851293

> Also, you shouldn't need hb_coretext_face_create() anymore. Right?

The code still uses it... it forms part of the caching code that is shared by the codepaths that don't have a CTFontRef (for FreeType/variable fonts?). They're unlikely to come up in UI code though (and we can just make a new codepath).

HarfBuzzFace.cpp in Blink is still using it. But if there's a performance advantage it would be good to cull from UI.

Long-term it would be good to merge a lot more with blink. For non-Mac, Blink's HarfBuzzFace cache looks much better optimised.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eff6931b804c3da1e76a2553f07a66ff0c8ace28

commit eff6931b804c3da1e76a2553f07a66ff0c8ace28
Author: Trent Apted <tapted@chromium.org>
Date: Mon Jan 08 03:14:54 2018

Reland: Mac RenderTextHarfBuzz: Use hb_coretext_font_create().

The HarfBuzz roll in r513541 improved Mac UI font support in the
renderer by detecting UI fonts and invoking CTFontCreateUIFontForLanguage().

However, in chrome UI, there is already a CTFont wrapped in a gfx::
PlatformFont for UI fonts. So creating a new CTFont for every hb_shape
call becomes unnecessary and expensive.

hb_coretext_font_create() (after a hb_coretext_face_create()) allows
the steps requiring the call to CTFontCreateUIFontForLanguage() to
be skipped by associating the CTFont with the hb_font_t directly.

Adds a new views_perftest target and LabelPerfTest.GetPreferredSize
to track. On a Late 2013 Mac Pro perf improves from 1390 runs/second
to 3300 runs/second.

+250 runs/second of that comes from a related hotspot in gfx::internal::
CreateSkiaTypeface() which was deriving a new CTFont unnecessarily
before sending it to gfx::CreateHarfBuzzFont().

Reland of r526920 with workarounds for some buggy system font data affecting
some low-priority corner cases on specific macOS versions.

Bug:  785522 , 799333
Change-Id: I02c6e47edfdd759fe477a61fe399a541a4a7ce3e
Reviewed-on: https://chromium-review.googlesource.com/851293
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527559}
[modify] https://crrev.com/eff6931b804c3da1e76a2553f07a66ff0c8ace28/ui/gfx/font.cc
[modify] https://crrev.com/eff6931b804c3da1e76a2553f07a66ff0c8ace28/ui/gfx/harfbuzz_font_skia.cc
[modify] https://crrev.com/eff6931b804c3da1e76a2553f07a66ff0c8ace28/ui/gfx/render_text_unittest.cc
[modify] https://crrev.com/eff6931b804c3da1e76a2553f07a66ff0c8ace28/ui/views/BUILD.gn
[modify] https://crrev.com/eff6931b804c3da1e76a2553f07a66ff0c8ace28/ui/views/DEPS
[add] https://crrev.com/eff6931b804c3da1e76a2553f07a66ff0c8ace28/ui/views/controls/label_perftest.cc
[add] https://crrev.com/eff6931b804c3da1e76a2553f07a66ff0c8ace28/ui/views/views_perftests.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b185243c6eea5e533f1871700ad68e9109a3f81c

commit b185243c6eea5e533f1871700ad68e9109a3f81c
Author: Trent Apted <tapted@chromium.org>
Date: Thu Jan 25 02:15:10 2018

Add views_perftests to the CQ and Waterfall (before adding to perf bots)

views_perftests run like cc_perftests. However, they need xvfb.

cc_perftests uses run_gtest_perf_test.py. However, its xvfb support has
been broken since r434762 which removed should_start_xvfb and other things.

run_telemetry_benchmark_as_googletest.py was fixed to cater for this
change in r491092. However, the same fix does not work for
run_gtest_perf_test.py because the helper in xvfb.py does not allow
stdout to redirect to a file, which is needed for the perf chart json
output.

Update xvfb.py to support this. However, having xvfb.py depend on
scripts/common.py creates a mess, so resolve by moving the
run_command_with_output() wrapper from testing/scripts/common.py to
testing/test_env.py. Nothing else is using it.

Start wrapping these test scripts in a proper gn target in testing/BUILD.gn
to avoid the same list appearing in dozens of places. However, this exposes
testing/BUILD.gn to the `gn check` graph, which requires fixing a silent
dependency that lots of test targets have on //testing/gmock_mutant.

Bug:  785522 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I413b642d53a8a6c8f8cc5326ff47e899b36abd37
Reviewed-on: https://chromium-review.googlesource.com/848466
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531789}
[modify] https://crrev.com/b185243c6eea5e533f1871700ad68e9109a3f81c/testing/BUILD.gn
[modify] https://crrev.com/b185243c6eea5e533f1871700ad68e9109a3f81c/testing/buildbot/chromium.clang.json
[modify] https://crrev.com/b185243c6eea5e533f1871700ad68e9109a3f81c/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/b185243c6eea5e533f1871700ad68e9109a3f81c/testing/buildbot/chromium.linux.json
[modify] https://crrev.com/b185243c6eea5e533f1871700ad68e9109a3f81c/testing/buildbot/chromium.mac.json
[modify] https://crrev.com/b185243c6eea5e533f1871700ad68e9109a3f81c/testing/buildbot/chromium.sandbox.json
[modify] https://crrev.com/b185243c6eea5e533f1871700ad68e9109a3f81c/testing/buildbot/chromium.win.json
[modify] https://crrev.com/b185243c6eea5e533f1871700ad68e9109a3f81c/testing/buildbot/gn_isolate_map.pyl
[modify] https://crrev.com/b185243c6eea5e533f1871700ad68e9109a3f81c/testing/buildbot/test_suites.pyl
[modify] https://crrev.com/b185243c6eea5e533f1871700ad68e9109a3f81c/testing/gmock/BUILD.gn
[modify] https://crrev.com/b185243c6eea5e533f1871700ad68e9109a3f81c/testing/scripts/common.py
[modify] https://crrev.com/b185243c6eea5e533f1871700ad68e9109a3f81c/testing/scripts/run_gtest_perf_test.py
[modify] https://crrev.com/b185243c6eea5e533f1871700ad68e9109a3f81c/testing/test_env.py
[modify] https://crrev.com/b185243c6eea5e533f1871700ad68e9109a3f81c/testing/xvfb.py
[modify] https://crrev.com/b185243c6eea5e533f1871700ad68e9109a3f81c/ui/views/BUILD.gn

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a99cab4fd72f4e71d628529b234de7407767aa43

commit a99cab4fd72f4e71d628529b234de7407767aa43
Author: Trent Apted <tapted@chromium.org>
Date: Fri Feb 09 00:21:38 2018

Add views_perftests to the perf waterfall

Bug:  785522 
Change-Id: I3754dd46768da80a3017f99460524c741015318e
Reviewed-on: https://chromium-review.googlesource.com/828226
Commit-Queue: Trent Apted <tapted@chromium.org>
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Reviewed-by: Emily Hanley <eyaich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535588}
[modify] https://crrev.com/a99cab4fd72f4e71d628529b234de7407767aa43/testing/buildbot/chromium.perf.json
[modify] https://crrev.com/a99cab4fd72f4e71d628529b234de7407767aa43/tools/perf/benchmark.csv
[modify] https://crrev.com/a99cab4fd72f4e71d628529b234de7407767aa43/tools/perf/core/perf_data_generator.py

http://go/views-perftests now links to graphs. Mac is still 6x slower than Windows on this perf test. It's pretty stable - I'll add some monitoring to pester me on regressions. Also a Linux chart isn't appearing. Anyway, I think this bug can be closed out.

Deleted: Screenshot from 2018-02-12 11-57-39.png 109 KB

	Screenshot from 2018-02-12 11-57-39.png 109 KB View Download