New issue
Advanced search Search tips

Issue 640466 link

Starred by 3 users

Issue metadata

Status: Assigned
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug


Sign in to add a comment

Tracking bug for issues related to AppKit running the main runloop in private modes

Project Member Reported by sdy@chromium.org, Aug 24 2016

Issue description

I've seen a couple of issues related to web pages freezing while AppKit does UI things. This bug is to help me keep track of them. My guts say that the right solution involves moving our own rendering off the main thread, but these guts are still young.
 

Comment 1 by sdy@chromium.org, Aug 24 2016

Description: Show this description

Comment 2 by sdy@chromium.org, Sep 26 2016

Labels: Hotlist-MacQualityOfLife

Comment 3 by sdy@chromium.org, Oct 5 2016

Labels: -Hotlist-MacQualityOfLife Proj-MacQualityOfLife

Comment 4 by rsesek@chromium.org, Oct 28 2016

Cc: rsesek@chromium.org

Comment 5 by rsesek@chromium.org, Oct 28 2016

Blockedon: 652007

Comment 6 by rsesek@chromium.org, Oct 28 2016

Blockedon: 651203

Comment 7 by rsesek@chromium.org, Oct 28 2016

Blocking: 651203

Comment 8 by rsesek@chromium.org, Oct 28 2016

Blockedon: -651203

Comment 9 by rsesek@chromium.org, Oct 28 2016

Blocking: 652007
Blockedon: -652007

Comment 11 by sdy@chromium.org, Dec 1 2016

Labels: -Proj-MacQualityOfLife Hotlist-MacQualityOfLife
Labels: -Hotlist-MacQualityOfLife Hotlist-PlatformExcellence
Migrating to more generic platform label, so that it can be applied to other platforms (i.e. I love the idea).

Comment 13 by sdy@chromium.org, Feb 23 2017

Blockedon: 651203

Comment 14 by sdy@chromium.org, Feb 23 2017

Blockedon: 604816 636290 652007
Blocking: -651203 -604816 -636290 -652007
I'm taking a bite - https://codereview.chromium.org/2852233002/
Project Member

Comment 16 by bugdroid1@chromium.org, May 11 2017

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

commit 335d78a0e0299bda3465cb7e303b79e043f680da
Author: tapted <tapted@chromium.org>
Date: Thu May 11 01:19:30 2017

Mac[Views]: Make native menus more responsive by pumping private runloop modes.

Native NSMenus run a UI-thread-blocking animation to flash the selected
item and fade out. This takes 300ms or more and events are only pumped
in private run loop modes. This can make menus feel really laggy -
<select> popups in particular.

A menu run by a native NSPopUpButton feels better since it uses a
different menu close animation which is opaque. Since it's opaque you
don't see how laggy the interface is behind it whilst the menu becomes
transparent. Using this animation is possible in MacViews, but its style
is fixed with a blue bar on the right, to match the native NSPopUpButton
appearance, so it looks weird. Changing any of the NSPopUpButtonCell's
style will just revert to a fade-out menu animation.

To get the menus feeling more responsive, "peek" at the processing on
the NSMenuItem by hooking -[NSMenuItem _sendItemSelectedNote]. This is
sent early in the event processing and before any animations start
blocking the UI thread. MenuController can peek at this and (optionally)
post the action handling as a task. Then, observe the private run loop
modes that AppKit uses during menu fade and "un" highlight of the menu
bar. This allows the posted task to execute and compositing tasks to run
in parallel with the menu fade.

BUG= 602914 , 640466
TEST=With chrome://flags#secondary-ui-md enabled, click the bookmark
star. Changing the Folder: should be immediately visible while the menu
is fading out.

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

[modify] https://crrev.com/335d78a0e0299bda3465cb7e303b79e043f680da/base/message_loop/message_pump_mac.mm
[modify] https://crrev.com/335d78a0e0299bda3465cb7e303b79e043f680da/chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm
[modify] https://crrev.com/335d78a0e0299bda3465cb7e303b79e043f680da/ui/base/cocoa/menu_controller.h
[modify] https://crrev.com/335d78a0e0299bda3465cb7e303b79e043f680da/ui/base/cocoa/menu_controller.mm
[modify] https://crrev.com/335d78a0e0299bda3465cb7e303b79e043f680da/ui/base/cocoa/menu_controller_unittest.mm
[modify] https://crrev.com/335d78a0e0299bda3465cb7e303b79e043f680da/ui/views/controls/menu/menu_runner_impl_cocoa.mm

r470769 is in Version 60.0.3097.0 Canary to play with. Nothing has exploded for me yet. Issue 604816 (youtube) and Issue 652007 (select+requestAnimationFrame) have good improvements. It does nothing for  Issue 636290  (sheet animation) or Issue 651203 (holding Cmd+w).
Project Member

Comment 18 by bugdroid1@chromium.org, May 20 2017

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

commit 80575da0e067c7c941edfd106287dd58ed578f8f
Author: tapted <tapted@chromium.org>
Date: Sat May 20 00:08:14 2017

Filter run loop modes in message_pump_mac.mm

There's a non-trivial overhead for the machinery to run posted tasks in
a given run loop mode. Only observe modes known to be run for a particular
thread.

BUG=640466,  724076 

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

[modify] https://crrev.com/80575da0e067c7c941edfd106287dd58ed578f8f/base/message_loop/message_pump_mac.h
[modify] https://crrev.com/80575da0e067c7c941edfd106287dd58ed578f8f/base/message_loop/message_pump_mac.mm

Blockedon: -651203 -604816 -636290 -652007 726200
Blocking: 651203 604816 636290 652007 602914
Cc: sdy@chromium.org
Labels: -Pri-3 M-61 Pri-2
Owner: tapted@chromium.org
Stealing your tracking bug since I've cited it in the commit log already :)

Plan is to disable r470769 for m60 though - see  Issue 726200 .
Project Member

Comment 20 by bugdroid1@chromium.org, May 25 2017

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

commit 08c4e3028cf0a15630a07874da6eec8694682ce4
Author: tapted <tapted@chromium.org>
Date: Thu May 25 10:43:53 2017

Mac: Disable r470769. I.e., Don't pump chrome tasks in private message loop modes.

It interacts badly with NSMenus on the native print dialog.

To fix, we may need to use a scoping class that adds these modes only
for menus that Chrome runs. Defer that to m61.

BUG= 726200 ,  602914 , 640466
TEST=Click the "Print using system dialog..." link in Print Preview,
then select "Save as PDF" via the [PDF] button in the native print
dialog. Chrome shouldn't hang.

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

[modify] https://crrev.com/08c4e3028cf0a15630a07874da6eec8694682ce4/base/message_loop/message_pump_mac.mm

Comment 21 by sdy@chromium.org, May 25 2017

How about pumping frames from a non-main thread?
Cc: ccameron@chromium.org
Frame pumping needs to coordinate with mouse events (and -[NSView setFrame] calls) during a window resize. A condition variable could make that work, but it will be complicated.

We also don't really know the capabilities of the RemoteFooSurfaceLayer APIs we're using to pump frames from the GPU. I.e. how thread-safe they are. This might be something ccameron has thought about already.

But also, for  Issue 602914  at least, the [menuItem action] still needs to be invoked before the frames will start pumping in the first place. And that must happen on the main UI thread, So, some combination will likely be necessary.

Comment 23 by sdy@chromium.org, May 26 2017

> Frame pumping needs to coordinate with mouse events (and -[NSView setFrame] calls) during a window resize.

This is kinda broken right now ( issue 617824 ), but that's a good point :/. Calling [CATransaction begin] and then [CATransaction end] (on the main thread, after receiving the frame) should work as well as things do now, if that's easier than a condition variable? Or, do we have the option of using a GCD(-style?) queue that could pump frames on a background thread normally, but also be pumped from the main thread (dispatch_sync(…) from -setFrame:)?

> But also, for  Issue 602914  at least, the [menuItem action] still needs to be invoked before the frames will start pumping in the first place. And that must happen on the main UI thread, So, some combination will likely be necessary.

I'm not 100% sure what you mean — input events would still be handled on the main thread, right? If frames are pumped on a background thread, it would even be OK if the close animation blocks the main thread after sending -action.
AppKit only invokes [item action] once all the private runloops it spins for animations have completed. But yeah - instead of posting a task when we "spy" on NSMenu to see what got clicked we could just process it synchronously.

Comment 25 by sdy@chromium.org, Aug 14 2017

Blocking: 635853
Project Member

Comment 26 by bugdroid1@chromium.org, Sep 20 2017

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

commit 24ab401b3662d052facf19a169f553ea9a6bf7e1
Author: Trent Apted <tapted@chromium.org>
Date: Wed Sep 20 11:34:22 2017

Re-enable listening on private runloop modes, for a whitelist.

r470769 started observing private AppKit run loop modes to keep
UI and web pages responsive while menus were fading out, but it
interacted badly with app-modal dialogs.

This re-enables r470769, but only in 3 places:
 - Web page context menus
 - Web page <select> menus
 - menus run from toolkit-views UI.

      pause briefly when either the <select> menu or the right-
      click context menu are fading out. (The main menubar and the
      Chrome app/wrench menu still cause a brief pause when dismissed).

Bug:  726200 ,  602914 , 640466, 652007
Test: https://jsfiddle.net/kvmy8q9d/5/ - the animation shouldn't
Change-Id: Idf769c1f830ccb3ddc67205acb9e86aac2ec0460
Reviewed-on: https://chromium-review.googlesource.com/647836
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503112}
[modify] https://crrev.com/24ab401b3662d052facf19a169f553ea9a6bf7e1/base/message_loop/message_pump_mac.h
[modify] https://crrev.com/24ab401b3662d052facf19a169f553ea9a6bf7e1/base/message_loop/message_pump_mac.mm
[modify] https://crrev.com/24ab401b3662d052facf19a169f553ea9a6bf7e1/base/message_loop/message_pump_mac_unittest.cc
[modify] https://crrev.com/24ab401b3662d052facf19a169f553ea9a6bf7e1/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
[modify] https://crrev.com/24ab401b3662d052facf19a169f553ea9a6bf7e1/content/browser/renderer_host/webmenurunner_mac.mm
[modify] https://crrev.com/24ab401b3662d052facf19a169f553ea9a6bf7e1/ui/views/controls/menu/menu_runner_impl_cocoa.mm

Blockedon: 24337
Blockedon: 873659 806162
Blockedon: -873659

Sign in to add a comment