New issue
Advanced search Search tips

Issue 605131 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug

Blocked on:
issue 615948



Sign in to add a comment

MacVIews: High CPU usage when scrolling table view

Project Member Reported by rsesek@chromium.org, Apr 20 2016

Issue description

Version: 52.0.2713.0
OS: 10.11.4

What steps will reproduce the problem?
(1) Enable --enable-mac-views-dialogs
(2) Open the task manager
(3) Scroll the table view fast
(4) Watch browser CPU spike between 75% - 100%

What is the expected output?
Less CPU usage.

What do you see instead?
More CPU usage.

Please use labels and text to provide additional information.

I've taken a screen recording and a sample. It looks like there's a hot spot in gfx::PlatformFontMac::DeriveFont. Perhaps we should cache the result inside views::TableView?
 
scroll-table-view.mov
9.1 MB Download
scroll-table-view-sample.txt
1.4 MB View Download
Labels: -Hotlist-MacViews Proj-MacViews
Cc: -tapted@chromium.org erikc...@chromium.org ellyjo...@chromium.org pinkerton@chromium.org ccameron@chromium.org
Labels: -Pri-2 Pri-1
Owner: tapted@chromium.org
Status: Assigned (was: Untriaged)
1. We should figure out why drawing text is so slow. 
2. Why is text drawing happening on the main thread, and the CPU rather than the GPU? [This is an open question, maybe this is the right thing to do] 
3. It would be really helpful if we added something like show-mac-overlay-borders for views so that we could tell which layers are being redrawn/damaged. It's theoretically possible that the reason the scrolling is janky is because we're redrawing everything for each frame, rather than just the newest cell, once [and then caching results, translating layers, etc.]
You can use QuartzDebug to flash the redraw areas which makes it really obvious if you're drawing too much during scrolling. 

Comment 4 by tapted@chromium.org, Apr 21 2016

DeriveFont! Yeah that should be easy to fix. Most places in views will cache FonLists. But it looks like there's one under RenderTextMac::DrawVisualText that's not getting cached.

It would be interesting to see if RenderTextHarfbuzz has a better caching policy. Although, I have a hunch it doesn't - e.g. views::Label caches the entire RenderText instance for performance, rather than just the font. So that also probably means this is an existing performance issue for all platforms using toolkit-views, not just Mac.

(thanks for the trace robert!)

And re #c2:
 (1): Drawing fonts is computationally heavy see e.g. Issue 441028
 (1a): Deriving fonts is also surprisingly intensive, but we can usually cache that.
 (2): Every views::View has a `ui::PaintCache paint_cache_;` data member -- fonts should be get painted into that, and it should only update when things are "invalidated" - scrolling currently invalidates more than it should AFAIK, but moving the elastic scrolling framework into src/ui should fix this. See e.g.  Issue 594907 
 (3) Yep - exactly! (that's  Issue 594907  too I guess). It's also easy just to layer-back a specific views::ScrollView in the current setup. AppKit tends to do this by default for you, and that will probably be the case in toolkit-views after adding elastic scrolling in.

Comment 5 by tapted@chromium.org, Apr 21 2016

Status: Started (was: Assigned)
RenderTextHarfbuzz does cache the SkTypeface. Doing the same for RenderTextMac is straightforward, and a nice cleanup - https://codereview.chromium.org/1907833002 .

The .Derive is actually a no-op regardless of the caching :/

RenderTextMac was doing:

CTFontRef = <get font from CTRun>
gfx::Font = <wrap CTFontRef>
style = <extract style from CTFontRef>
gfx::Font_2 = apply <style> to gfx::Font  # This is a no-op

Project Member

Comment 6 by bugdroid1@chromium.org, Apr 22 2016

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

commit 758d2baa307aa32b9de708e97655663726b8fa9c
Author: tapted <tapted@chromium.org>
Date: Fri Apr 22 06:19:08 2016

RenderTextMac: Cache the SkTypeface on the TextRun

SkiaTextRenderer::SetFontWithStyle() uses gfx::Font::Derive() which is
slow. SetFontWithStyle is only used on Mac but, also, there was actually
never anything to derive on Mac, since RenderTextMac does (on every
*paint*):
  CTFontRef = <get font from CTRun>
  gfx::Font = <wrap CTFontRef>
  CFFontRef = <unwrap from gfx:Font>  # cheap
  style = <extract style from CTFontRef>
  gfx::Font_2 = Derive(apply <style> to gfx::Font)  # A "no-op", but expensive

So remove SkiaTextRenderer::SetFontWithStyle() since nothing needs it.

Then store the SkTypeface on the RenderTextMac TextRun. This saves a
lookup on each paint and is consistent with RenderTextHarfbuzz.

BUG= 605131 ,  605136 

Review URL: https://codereview.chromium.org/1907833002

Cr-Commit-Position: refs/heads/master@{#389042}

[modify] https://crrev.com/758d2baa307aa32b9de708e97655663726b8fa9c/ui/gfx/render_text.cc
[modify] https://crrev.com/758d2baa307aa32b9de708e97655663726b8fa9c/ui/gfx/render_text.h
[modify] https://crrev.com/758d2baa307aa32b9de708e97655663726b8fa9c/ui/gfx/render_text_harfbuzz.cc
[modify] https://crrev.com/758d2baa307aa32b9de708e97655663726b8fa9c/ui/gfx/render_text_harfbuzz.h
[modify] https://crrev.com/758d2baa307aa32b9de708e97655663726b8fa9c/ui/gfx/render_text_mac.h
[modify] https://crrev.com/758d2baa307aa32b9de708e97655663726b8fa9c/ui/gfx/render_text_mac.mm

Project Member

Comment 7 by bugdroid1@chromium.org, Apr 22 2016

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

commit 6371bc49fe4c8e605a1b0a98e417e6aea80a5168
Author: kjellander <kjellander@chromium.org>
Date: Fri Apr 22 11:24:01 2016

Revert of RenderTextMac: Cache the SkTypeface on the TextRun (patchset #4 id:80001 of https://codereview.chromium.org/1907833002/ )

Reason for revert:
Seems to break gfx_unittests on Mac10.11 Tests: https://build.chromium.org/p/chromium.mac/builders/Mac10.11%20Tests/builds/1384

See
https://findit-for-me.appspot.com/build-failure?url=https://build.chromium.org/p/chromium.mac/builders/Mac10.11%20Tests/builds/1384 for more info.

Original issue's description:
> RenderTextMac: Cache the SkTypeface on the TextRun
>
> SkiaTextRenderer::SetFontWithStyle() uses gfx::Font::Derive() which is
> slow. SetFontWithStyle is only used on Mac but, also, there was actually
> never anything to derive on Mac, since RenderTextMac does (on every
> *paint*):
>   CTFontRef = <get font from CTRun>
>   gfx::Font = <wrap CTFontRef>
>   CFFontRef = <unwrap from gfx:Font>  # cheap
>   style = <extract style from CTFontRef>
>   gfx::Font_2 = Derive(apply <style> to gfx::Font)  # A "no-op", but expensive
>
> So remove SkiaTextRenderer::SetFontWithStyle() since nothing needs it.
>
> Then store the SkTypeface on the RenderTextMac TextRun. This saves a
> lookup on each paint and is consistent with RenderTextHarfbuzz.
>
> BUG= 605131 ,  605136 

TBR=asvitkine@chromium.org,tapted@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 605131 ,  605136 

Review URL: https://codereview.chromium.org/1914443002

Cr-Commit-Position: refs/heads/master@{#389076}

[modify] https://crrev.com/6371bc49fe4c8e605a1b0a98e417e6aea80a5168/ui/gfx/render_text.cc
[modify] https://crrev.com/6371bc49fe4c8e605a1b0a98e417e6aea80a5168/ui/gfx/render_text.h
[modify] https://crrev.com/6371bc49fe4c8e605a1b0a98e417e6aea80a5168/ui/gfx/render_text_harfbuzz.cc
[modify] https://crrev.com/6371bc49fe4c8e605a1b0a98e417e6aea80a5168/ui/gfx/render_text_harfbuzz.h
[modify] https://crrev.com/6371bc49fe4c8e605a1b0a98e417e6aea80a5168/ui/gfx/render_text_mac.h
[modify] https://crrev.com/6371bc49fe4c8e605a1b0a98e417e6aea80a5168/ui/gfx/render_text_mac.mm

Project Member

Comment 8 by bugdroid1@chromium.org, Apr 26 2016

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

commit 59fe54df8c0de55f03c8fb5e1860279d2993b473
Author: tapted <tapted@chromium.org>
Date: Tue Apr 26 22:38:09 2016

RenderTextMac: Cache the SkTypeface on the TextRun [reland]

SkiaTextRenderer::SetFontWithStyle() uses gfx::Font::Derive() which is
slow. SetFontWithStyle is only used on Mac but, also, there was actually
never anything to derive on Mac, since RenderTextMac does (on every
*paint*):
  CTFontRef = <get font from CTRun>
  gfx::Font = <wrap CTFontRef>
  CFFontRef = <unwrap from gfx:Font>  # cheap
  style = <extract style from CTFontRef>
  gfx::Font_2 = Derive(apply <style> to gfx::Font)  # A "no-op", but expensive

So remove SkiaTextRenderer::SetFontWithStyle() since nothing needs it.

Then store the SkTypeface on the RenderTextMac TextRun. This saves a
lookup on each paint and is consistent with RenderTextHarfbuzz.

[reland: prior CL regressed RenderTextTest.Multiline_ZeroWidthChars]

BUG= 605131 ,  605136 

Review URL: https://codereview.chromium.org/1919123002

Cr-Commit-Position: refs/heads/master@{#389924}

[modify] https://crrev.com/59fe54df8c0de55f03c8fb5e1860279d2993b473/ui/gfx/render_text.cc
[modify] https://crrev.com/59fe54df8c0de55f03c8fb5e1860279d2993b473/ui/gfx/render_text.h
[modify] https://crrev.com/59fe54df8c0de55f03c8fb5e1860279d2993b473/ui/gfx/render_text_harfbuzz.cc
[modify] https://crrev.com/59fe54df8c0de55f03c8fb5e1860279d2993b473/ui/gfx/render_text_harfbuzz.h
[modify] https://crrev.com/59fe54df8c0de55f03c8fb5e1860279d2993b473/ui/gfx/render_text_mac.h
[modify] https://crrev.com/59fe54df8c0de55f03c8fb5e1860279d2993b473/ui/gfx/render_text_mac.mm
[modify] https://crrev.com/59fe54df8c0de55f03c8fb5e1860279d2993b473/ui/gfx/render_text_unittest.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Apr 29 2016

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

commit 45d205ff3d72b742b3c3d16c66d6074800e86baf
Author: tapted <tapted@chromium.org>
Date: Fri Apr 29 01:22:21 2016

RenderText: Don't default-construct gfx::Fonts unnecessarily

The default constructor for gfx::Font is expensive. Don't use it when
creating text runs. Instead, reference an existing font.

RenderTextMac doesn't need the gfx::Font at all, so just retain the
CTFont on the run, since constructing a gfx::Font from an NSFont is also
expensive.

BUG= 605131 

Review-Url: https://codereview.chromium.org/1925033002
Cr-Commit-Position: refs/heads/master@{#390556}

[modify] https://crrev.com/45d205ff3d72b742b3c3d16c66d6074800e86baf/ui/gfx/render_text_harfbuzz.cc
[modify] https://crrev.com/45d205ff3d72b742b3c3d16c66d6074800e86baf/ui/gfx/render_text_harfbuzz.h
[modify] https://crrev.com/45d205ff3d72b742b3c3d16c66d6074800e86baf/ui/gfx/render_text_mac.h
[modify] https://crrev.com/45d205ff3d72b742b3c3d16c66d6074800e86baf/ui/gfx/render_text_mac.mm
[modify] https://crrev.com/45d205ff3d72b742b3c3d16c66d6074800e86baf/ui/gfx/render_text_unittest.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Apr 29 2016

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

commit 8967fcb75d4fd8bfcaca2db2a8db2931a16e55ba
Author: tapted <tapted@chromium.org>
Date: Fri Apr 29 03:53:38 2016

Remove calls to NSClassFromString while casting NSFont via foundation_util

A performance bottleneck in RenderTextMac flagged calls to
NSClassFromString(@"NSFont") as being surprisingly expensive.

Use [NSFont class] instead.

BUG= 605131 

Review-Url: https://codereview.chromium.org/1928233002
Cr-Commit-Position: refs/heads/master@{#390589}

[modify] https://crrev.com/8967fcb75d4fd8bfcaca2db2a8db2931a16e55ba/base/mac/foundation_util.mm

Status: Assigned (was: Started)
Testing 52.0.2721.0 I think this is now on par with the Cocoa task manager.

My highly scientific, "Open 20 tabs, a task manager, then spin the scroll wheel up and down as fast as I can" takes the CPU usage of the browser process up to about 30% on my trashcan with both Cocoa and toolkit-views task managers.

Attaching a trace as well -- a lot of the main culprits are in CoreText, which will be tricky to improve upon. I'll re-do the analysis if it looks like we're about to switch over to Harfbuzz, but we're now in a much better shape to do an apples-to-apples performance comparison of RenderTextHarfbuzz and RenderTextMac.

The only other low-hanging fruit I see are perhaps some malloc calls around std::vector that could possibly be avoided with some judicious use of reserve() or a different collection classes. (e.g. stuff around gfx::internal::StyleIterator ).

Also gfx::ElideText seems kinda expensive. It spawns a separate, but complete RenderTextMac to do its calculations. A lighter-weight RenderText class might be possible if we're just eliding text rather than drawing it.

Putting this on the back-burner for now.
Sample of Google Chrome-52.0.2721.0.txt
730 KB View Download
Blockedon: 615948
Project Member

Comment 13 by bugdroid1@chromium.org, Aug 5 2016

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

commit b7720c4c053a09050c9abab73b0af528150dd42c
Author: tapted <tapted@chromium.org>
Date: Fri Aug 05 07:17:15 2016

MacViews: Claim -[NSView isOpaque] for the content view according to the root layer properties.

Once we start scrolling with Layers, most of the CPU taken up when
scrolling is actually taken up by redrawing the window frame. This is
because BridgedContentView currently always claims that it is
transparent. So, when paint areas are invalidated, AppKit must first ask
the window frame to redraw that region in case BridgedContentView wants
to draw something transparent on top of that.

For opaque windows, none of the window background shows through, so
report YES from -[NSView isOpaque] in those cases.

BUG= 594907 ,  605131 

Review-Url: https://codereview.chromium.org/2219663003
Cr-Commit-Position: refs/heads/master@{#410012}

[modify] https://crrev.com/b7720c4c053a09050c9abab73b0af528150dd42c/ui/views/cocoa/bridged_content_view.mm
[modify] https://crrev.com/b7720c4c053a09050c9abab73b0af528150dd42c/ui/views/widget/native_widget_mac_unittest.mm

Project Member

Comment 14 by bugdroid1@chromium.org, Aug 13 2016

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

commit 4b70c528d7a6652e534bc5d9efd19d2d57d3fe4e
Author: tapted <tapted@chromium.org>
Date: Sat Aug 13 09:09:32 2016

Scroll with Layers in views::ScrollView

Initially, do this only on Mac, or with
--enable-features=ToolkitViewsScrollWithLayers.

This is a first step for bringing up elastic scrolling of a
views::ScrollView on Mac. Still, by itself, it results in significant
performance improvements for scrolling in toolkit-views and allows the
UI thread to keep pace with the high rate of touchpad scroll events
produced by Cocoa on Mac.

To take advantage of existing scroll handling for smooth-scrolling and
elasticity, later CLs will route scroll events through the
cc::InputHandler (i.e. LayerTreeHostImpl). Since the ui::Compositor is
single-threaded on the UI thread, there is no distinct impl side. For a
consistent event flow, perform all scrolling on the impl side. This
requires plumbing through a way for a views::ScrollView to set the
scroll offset on the impl side directly, in response to an input event.

views::ScrollView now makes its contents layer-backed and clips
it to the layer added to the existing ScrollView viewport.

BUG=615948,  594907 ,  605131 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2188133002
Cr-Commit-Position: refs/heads/master@{#411889}

[modify] https://crrev.com/4b70c528d7a6652e534bc5d9efd19d2d57d3fe4e/cc/input/input_handler.h
[modify] https://crrev.com/4b70c528d7a6652e534bc5d9efd19d2d57d3fe4e/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/4b70c528d7a6652e534bc5d9efd19d2d57d3fe4e/cc/trees/layer_tree_host_impl.h
[modify] https://crrev.com/4b70c528d7a6652e534bc5d9efd19d2d57d3fe4e/cc/trees/single_thread_proxy.cc
[modify] https://crrev.com/4b70c528d7a6652e534bc5d9efd19d2d57d3fe4e/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
[modify] https://crrev.com/4b70c528d7a6652e534bc5d9efd19d2d57d3fe4e/chrome/browser/ui/views/extensions/extension_install_dialog_view.h
[modify] https://crrev.com/4b70c528d7a6652e534bc5d9efd19d2d57d3fe4e/ui/compositor/compositor.cc
[modify] https://crrev.com/4b70c528d7a6652e534bc5d9efd19d2d57d3fe4e/ui/compositor/compositor.h
[modify] https://crrev.com/4b70c528d7a6652e534bc5d9efd19d2d57d3fe4e/ui/compositor/layer.cc
[modify] https://crrev.com/4b70c528d7a6652e534bc5d9efd19d2d57d3fe4e/ui/compositor/layer.h
[modify] https://crrev.com/4b70c528d7a6652e534bc5d9efd19d2d57d3fe4e/ui/events/blink/input_handler_proxy_unittest.cc
[modify] https://crrev.com/4b70c528d7a6652e534bc5d9efd19d2d57d3fe4e/ui/views/controls/scroll_view.cc
[modify] https://crrev.com/4b70c528d7a6652e534bc5d9efd19d2d57d3fe4e/ui/views/controls/scroll_view.h
[modify] https://crrev.com/4b70c528d7a6652e534bc5d9efd19d2d57d3fe4e/ui/views/controls/scroll_view_unittest.cc
[modify] https://crrev.com/4b70c528d7a6652e534bc5d9efd19d2d57d3fe4e/ui/views/view.cc

Status: Fixed (was: Assigned)
Elastic scrolling still needs to be plumbed in, but this probably isn't going to get much better (nor worse I expect, once elasticity is plumbed). Rough estimates with r411889 in Canary suggests CPU now spikes to significantly less CPU than Cocoa. E.g. (scrolling up & down):
 wheel mouse: ~8% CPU views, ~40% CPU Cocoa
 touchpad: ~10% CPU views, ~60% CPU Cocoa

Currently the views::TableView still repaints the _header_ without layers. So if there's horizontal scrolling in the mix, the header row gets damaged needs a repaint. But you have to drag your column widths to get into a state where horizontal scrolling happens, and the stuff to repaint has a reasonable upper bound, so that probably isn't worth optimizing for.

Sign in to add a comment