MacViewsBrowser: Bookmark menu mouse tracking is janktastic |
||||||||||||
Issue descriptionChrome Version: 67.0.3381.0 OS: macOS 10.13.3 What steps will reproduce the problem? (1) --enable-features=ViewsBrowserWindows (2) Create a folder in the bookmark bar and add a few bookmarks to it (3) Open the menu (4) Scrub the mouse over the menu items (5) Watch as the menu item highlight fails to keep up with the mouse What is the expected result? The highlighted menu item should follow the mouse quickly and smoothly. What happens instead? Jank Please use labels and text to provide additional information. For graphics-related bugs, please copy/paste the contents of the about:gpu page at the end of this report.
,
Mar 29 2018
** Bulk Edit ** FYI: Starting 04/13 M68 will be in canary, M68 Dev promotion will be on 04/26.
,
Apr 25 2018
Pls mark the bug as fixed if CL is landed in trunk and nothing else is pending. Thank you.
,
Jun 14 2018
,
Jun 20 2018
,
Jun 25 2018
See also Issue 847710 . I think there are two avenues to explore: - Don't invoke NSAccessibilityPostNotificationForObservedElementWithUserInfo unless VoiceOver is on - Cache tooltips in BookmarkBarView::CreateToolTipForURLAndTitle
,
Jun 25 2018
,
Jun 25 2018
Caching the tooltips seems pretty reasonable. Otherwise I worry that the VoiceOver experience could suffer from the same issue.
,
Jul 1
I summed together the various stacks hitting gfx::RenderTextHarfBuzz::ShapeRunWithFont, and they account for ~80% of the CPU usage. gfx::RenderTextHarfBuzz::ShapeRunWithFont has little state (umm, comparatively) -- I'd advocate adding a cache for the result of calls to ShapeRunWithFont (maybe in TLS, maybe just for the browser UI thread, maybe just for mac). The following super-hacky cache gets us back to 60fps (CPU usage is at ~45% while moving the mouse as vigorously as I can). That way, we'll salvage all callers to this path (well, the ones that don't use new text every time...). https://chromium-review.googlesource.com/c/chromium/src/+/1121820
,
Jul 2
Nice find ccameron! Over to you :)
,
Jul 2
,
Jul 2
Re #12 -- let me know how that goes -- ideally we wouldn't need to cache the results because the calls to HarfBuzz wouldn't be that expensive.
,
Jul 2
Short version is I tried to back this change out https://github.com/harfbuzz/harfbuzz/commit/4acce77db7dd588ba277779c4997b0256ebe426e and set the embedding at the attributed string level. Visually, it looked fine, but I was seeing stuff like [38623:775:0702/143430.556396:ERROR:render_text_harfbuzz.cc(732)] TextRunHarfBuzz error, please report at crbug.com/724880 : range: {0,3}, rtl: 0, level: '', script: 0, font: '.AppleSystemUIFont', glyph_count: 3, pos: 0, glyph_to_char: 0->2, 1->1, 2->0, in the console, so I think it's not going to fly. +drott@ is there some other way we can set the embedding without having to pay the cost of constructing the typesetter?
,
Jul 3
I think I am lacking some context here. Are you talking about setting Bidi LTR/RTL embedding? I am not sure I understand where reshape requests are coming from. Before you go for a cache, are there ways to avoid hitting the shaper repeatedly? What CoreText usage (hit through hb_shape) are you trying to control/reduce? CC'ing Behdad.
,
Jul 3
Sorry, the context is: We're getting ready to ship MacViews browser, but we're seeing performance regressions in a bunch of unrelated UI surfaces across multiple systems (layout, paint, a11y) that bottom out at text shaping. We're working on adding caching and reducing calls, but these are code paths that have been in production on Windows and Linux for a while, so another avenue we're pursuing is why Mac is seeing unacceptable performance but other platforms aren't. One particular issue we've seen via profiling is that _hb_coretext_shape creates a CTTypesetter to set a BiDi embedding level depending on the direction of the buffer. This looks like it's particularly slow: for example, in c#12, you can see an Instruments trace of hovering over the tab strip. Over 25% of samples are spent creating a typesetter. I tried a local patch to avoid the typesetter by wrapping the entire string to be shaped in a CTWritingDirection attribute, which seemed OK in a cursory visual inspection, but threw errors on the RenderTextHarfBuzz side, so it must have not been equivalent. What we'd like to know is if there's some other change that could be made on the HarfBuzz side to make this call less expensive.
,
Jul 3
> so another avenue we're pursuing is why Mac is seeing unacceptable performance but other platforms aren't. Perhaps you're aware of this already. As background: On Mac, for AAT fonts (fonts that have Apple Advanced typography features, such as a trak OpenType table), HarfBuzz choses the hb_coretext backend, which means it ultimately shapes text with the help of CoreText. This is because HarfBuzz itself cannot handle some AAT features yet. The trak table is important for the system UI fonts such as .AppleSystemUIFont and .NSSFDisplay etc. The performance of this backend is not directly comparable to what HarfBuzz does on its own, in the hb-opentype shaping backend, where HarfBuzz does all work internally. > One particular issue we've seen via profiling is that _hb_coretext_shape creates a CTTypesetter to set a BiDi embedding level depending on the direction of the buffer. This looks like it's particularly slow: for example, in c#12, you can see an Instruments trace of hovering over the tab strip. Over 25% of samples are spent creating a typesetter. I think hb-coretext always sets up a CoreText CTTypeSetter, not only when configuring a Bidi direction, and I don't think there is much we can do about dropping this as this is the general way HarfBuzz invokes CoreText shaping. Did you see big performance improvements with the attempted change to move from CTTypeSetter to CTLine? I'd leave it for Behdad to comment. There are long term plans to incorporate full AAT support into HarfBuzz itself, which may have better performance than the hb-coretext backend, as there is less interface friction and potentially a faster implementation.
,
Jul 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/79c4dc9d0916a74430ce84c2d39d8360ca3a902a commit 79c4dc9d0916a74430ce84c2d39d8360ca3a902a Author: Christopher Cameron <ccameron@chromium.org> Date: Wed Jul 04 01:04:44 2018 Views: Add MRUCache of results from ShapeRunWithFont Bookmark folders using Views are very janky (~5 fps, with the CPU at 100%). Calls to RenderTextHarfBuzz::ShapeRunWithFont account for almost all of this time. Many of the calls to ShapeRunWithFont are done with the same arguments every frame. To alleviate some of this jank, add a cache the results of this function call. Refactor RenderTextHarfBuzz::ShapeRunWithFont to be stateless, and add structures ShapeRunWithFontInput and ShapeRunWithFontOutput for the arguments and output of the function. Use ShapeRunWithFontInput as a cache key. Bug: 826265 Change-Id: Ic6726c1152c44dc5cb0eccddf1ff3adcd060d8e7 Reviewed-on: https://chromium-review.googlesource.com/1123456 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: Trent Apted <tapted@chromium.org> Cr-Commit-Position: refs/heads/master@{#572420} [modify] https://crrev.com/79c4dc9d0916a74430ce84c2d39d8360ca3a902a/ui/gfx/font_render_params.cc [modify] https://crrev.com/79c4dc9d0916a74430ce84c2d39d8360ca3a902a/ui/gfx/font_render_params.h [modify] https://crrev.com/79c4dc9d0916a74430ce84c2d39d8360ca3a902a/ui/gfx/render_text_harfbuzz.cc [modify] https://crrev.com/79c4dc9d0916a74430ce84c2d39d8360ca3a902a/ui/gfx/render_text_harfbuzz.h [modify] https://crrev.com/79c4dc9d0916a74430ce84c2d39d8360ca3a902a/ui/gfx/render_text_unittest.cc
,
Jul 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1a5890105a339862a64acd6a1ffa5eb211679319 commit 1a5890105a339862a64acd6a1ffa5eb211679319 Author: Christopher Cameron <ccameron@chromium.org> Date: Wed Jul 04 05:42:43 2018 Views: Add MRUCache of results from ShapeRunWithFont Add missing patchset from crrev.com/572420 Bug: 826265 Change-Id: I2506916da8a489c83f3c93ff3a143070505c3bba Reviewed-on: https://chromium-review.googlesource.com/1125528 Reviewed-by: Trent Apted <tapted@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#572487} [modify] https://crrev.com/1a5890105a339862a64acd6a1ffa5eb211679319/ui/gfx/render_text_harfbuzz.cc
,
Jul 12
,
Jul 12
,
Jul 12
More RenderTextHarfbuzz discussion over in issue 862773
,
Jul 12
Marking this as fixed because the menus themselves are decent now. The general "RenderTextHarfbuzz is blockshippably slow" is over in issue 862773. |
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by ellyjo...@chromium.org
, Mar 29 2018Owner: ellyjo...@chromium.org
Status: Assigned (was: Untriaged)