MacVIews: High CPU usage when scrolling table view |
||||||
Issue descriptionVersion: 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?
,
Apr 20 2016
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.]
,
Apr 20 2016
You can use QuartzDebug to flash the redraw areas which makes it really obvious if you're drawing too much during scrolling.
,
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.
,
Apr 21 2016
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
,
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
,
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
,
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
,
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
,
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
,
May 2 2016
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.
,
May 31 2016
,
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
,
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
,
Aug 15 2016
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 |
||||||
Comment 1 by lafo...@chromium.org
, Apr 20 2016