Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
331513002:
[Mac] Use a native MessagePump instead of a MessagePumpDefault (Closed)
|
Created:
6 years, 6 months ago by jeremy Modified:
6 years, 6 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, sadrul, tonyg Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[Mac] Use a native MessagePump instead of a MessagePumpDefault
Many message loops in Chrome are backed by MessagePumpDefault which uses a WaitableEvent object to sleep, this is ultimately implemented as a pthread condition variable on POSIX.
In order to support timer coalescing in MessageLoops we need to sleep using a primitive which supports timer coalescing at the OS level (support for timer coalescing goes down into the kernel on Android/Linux/OSX/Win). The current change achieves this and should provide a way to gauge the effect of timer coalescing.
An alternate approach to this patch would be to implement MessagePumpDefault using a primitive which supports timer coalescing.
Local performance tests didn't uncover any regressions from this change.
BUG=356804
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276808
Patch Set 1 #
Total comments: 1
Patch Set 2 : Change to use a MessagePumpCFRunLoop #
Total comments: 1
Messages
Total messages: 21 (0 generated)
Scary, but LGTM in a “let’s try and see what happens” sort of way. Consider the comment for further investigation. https://codereview.chromium.org/331513002/diff/1/base/message_loop/message_lo... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/331513002/diff/1/base/message_loop/message_lo... base/message_loop/message_loop.cc:234: #define MESSAGE_PUMP_DEFAULT scoped_ptr<MessagePump>(MessagePumpMac::Create()) We may want a MessagePumpCFRunLoop instead of a MessagePumpNSRunLoop. I don’t think we’ve ever gauged performance. You should look into this.
The CQ bit was checked by jeremy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremy@chromium.org/331513002/20001
I'm a little nervous about this change. There is a bug open to actually stop using NSRunLoop in the renderer process. See: https://code.google.com/p/chromium/issues/detail?id=306348 Can we find a way to avoid this? -Darin On Wed, Jun 11, 2014 at 3:46 PM, <commit-bot@chromium.org> wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/jeremy@ > chromium.org/331513002/20001 > > > https://codereview.chromium.org/331513002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
What does "an alternate approach to this patch would be to implement MessagePumpDefault using a primitive which supports timer coalescing" mean? On Wed, Jun 11, 2014 at 5:05 PM, Darin Fisher <darin@chromium.org> wrote: > I'm a little nervous about this change. There is a bug open to actually > stop using NSRunLoop in the renderer process. > > See: > https://code.google.com/p/chromium/issues/detail?id=306348 > > Can we find a way to avoid this? > > -Darin > > > On Wed, Jun 11, 2014 at 3:46 PM, <commit-bot@chromium.org> wrote: > >> CQ is trying da patch. Follow status at >> https://chromium-status.appspot.com/cq/jeremy@ >> chromium.org/331513002/20001 >> >> >> https://codereview.chromium.org/331513002/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15742) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18874)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
The NSRunLoop shouldn’t be harmful in the renderers. It was important to get rid of the NSApplication run loop because that reached out to the system in undesirable ways, but an NSRunLoop and CFRunLoop are basically just loops. If we don’t actually use any of the features provided by NSRunLoop/CFRunLoop then I agree that we should move off of them and on to MessagePumpDefault for the renderer’s main loop, but if we’re getting something of value from them, then they shouldn’t cause any trouble. On Wed, Jun 11, 2014 at 8:05 PM, Darin Fisher <darin@chromium.org> wrote: > I'm a little nervous about this change. There is a bug open to actually > stop using NSRunLoop in the renderer process. > > See: > https://code.google.com/p/chromium/issues/detail?id=306348 > > Can we find a way to avoid this? > > -Darin > > > On Wed, Jun 11, 2014 at 3:46 PM, <commit-bot@chromium.org> wrote: > >> CQ is trying da patch. Follow status at >> https://chromium-status.appspot.com/cq/jeremy@ >> chromium.org/331513002/20001 >> >> >> https://codereview.chromium.org/331513002/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Background: My goal is to move most of our threads to be based on MessagePumpMojo. That blocks on a call to MojoWaitMany (essentially just a cvar under the hood). The idea with that change is to minimize signalling latency for consumers of Mojo interfaces. Otherwise, we'll have an extra PostTask to deliver such signals back to the thread of the consumers. -Darin On Wed, Jun 11, 2014 at 6:55 PM, Mark Mentovai <mark@chromium.org> wrote: > The NSRunLoop shouldn’t be harmful in the renderers. It was important to > get rid of the NSApplication run loop because that reached out to the > system in undesirable ways, but an NSRunLoop and CFRunLoop are basically > just loops. > > If we don’t actually use any of the features provided by > NSRunLoop/CFRunLoop then I agree that we should move off of them and on to > MessagePumpDefault for the renderer’s main loop, but if we’re getting > something of value from them, then they shouldn’t cause any trouble. > > > On Wed, Jun 11, 2014 at 8:05 PM, Darin Fisher <darin@chromium.org> wrote: > >> I'm a little nervous about this change. There is a bug open to actually >> stop using NSRunLoop in the renderer process. >> >> See: >> https://code.google.com/p/chromium/issues/detail?id=306348 >> >> Can we find a way to avoid this? >> >> -Darin >> >> >> On Wed, Jun 11, 2014 at 3:46 PM, <commit-bot@chromium.org> wrote: >> >>> CQ is trying da patch. Follow status at >>> https://chromium-status.appspot.com/cq/jeremy@ >>> chromium.org/331513002/20001 >>> >>> >>> https://codereview.chromium.org/331513002/ >>> >> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Interesting. How do timers work in MessagePumpMojo then? On Wed, Jun 11, 2014 at 11:11 PM, Darin Fisher <darin@chromium.org> wrote: > Background: > > My goal is to move most of our threads to be based on MessagePumpMojo. > That blocks on a call to MojoWaitMany (essentially just a cvar under the > hood). The idea with that change is to minimize signalling latency for > consumers of Mojo interfaces. Otherwise, we'll have an extra PostTask to > deliver such signals back to the thread of the consumers. > > -Darin > > > On Wed, Jun 11, 2014 at 6:55 PM, Mark Mentovai <mark@chromium.org> wrote: > >> The NSRunLoop shouldn’t be harmful in the renderers. It was important to >> get rid of the NSApplication run loop because that reached out to the >> system in undesirable ways, but an NSRunLoop and CFRunLoop are basically >> just loops. >> >> If we don’t actually use any of the features provided by >> NSRunLoop/CFRunLoop then I agree that we should move off of them and on to >> MessagePumpDefault for the renderer’s main loop, but if we’re getting >> something of value from them, then they shouldn’t cause any trouble. >> >> >> On Wed, Jun 11, 2014 at 8:05 PM, Darin Fisher <darin@chromium.org> wrote: >> >>> I'm a little nervous about this change. There is a bug open to actually >>> stop using NSRunLoop in the renderer process. >>> >>> See: >>> https://code.google.com/p/chromium/issues/detail?id=306348 >>> >>> Can we find a way to avoid this? >>> >>> -Darin >>> >>> >>> On Wed, Jun 11, 2014 at 3:46 PM, <commit-bot@chromium.org> wrote: >>> >>>> CQ is trying da patch. Follow status at >>>> https://chromium-status.appspot.com/cq/jeremy@ >>>> chromium.org/331513002/20001 >>>> >>>> >>>> https://codereview.chromium.org/331513002/ >>>> >>> >>> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
MojoWaitMany takes a timeout. It is similar to poll, select, WaitForMultipleObjects, etc. -Darin On Wed, Jun 11, 2014 at 8:50 PM, Mark Mentovai <mark@chromium.org> wrote: > Interesting. How do timers work in MessagePumpMojo then? > > > On Wed, Jun 11, 2014 at 11:11 PM, Darin Fisher <darin@chromium.org> wrote: > >> Background: >> >> My goal is to move most of our threads to be based on MessagePumpMojo. >> That blocks on a call to MojoWaitMany (essentially just a cvar under the >> hood). The idea with that change is to minimize signalling latency for >> consumers of Mojo interfaces. Otherwise, we'll have an extra PostTask to >> deliver such signals back to the thread of the consumers. >> >> -Darin >> >> >> On Wed, Jun 11, 2014 at 6:55 PM, Mark Mentovai <mark@chromium.org> wrote: >> >>> The NSRunLoop shouldn’t be harmful in the renderers. It was important to >>> get rid of the NSApplication run loop because that reached out to the >>> system in undesirable ways, but an NSRunLoop and CFRunLoop are basically >>> just loops. >>> >>> If we don’t actually use any of the features provided by >>> NSRunLoop/CFRunLoop then I agree that we should move off of them and on to >>> MessagePumpDefault for the renderer’s main loop, but if we’re getting >>> something of value from them, then they shouldn’t cause any trouble. >>> >>> >>> On Wed, Jun 11, 2014 at 8:05 PM, Darin Fisher <darin@chromium.org> >>> wrote: >>> >>>> I'm a little nervous about this change. There is a bug open to actually >>>> stop using NSRunLoop in the renderer process. >>>> >>>> See: >>>> https://code.google.com/p/chromium/issues/detail?id=306348 >>>> >>>> Can we find a way to avoid this? >>>> >>>> -Darin >>>> >>>> >>>> On Wed, Jun 11, 2014 at 3:46 PM, <commit-bot@chromium.org> wrote: >>>> >>>>> CQ is trying da patch. Follow status at >>>>> https://chromium-status.appspot.com/cq/jeremy@ >>>>> chromium.org/331513002/20001 >>>>> >>>>> >>>>> https://codereview.chromium.org/331513002/ >>>>> >>>> >>>> >>> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
As Mark mentions this change is quite minimal and isolated and doesn't increase our reliance on Mac APIs. I'd like to land this in order to gauge the benefit we can get from coalescing timers in the browser process esp. if this can inform us about how we want to implement the Mojo primitives. If we can reduce the wakeups significantly without hurting performance then it makes sense to use APIs that support timer coalescing in Mojo. If it turns out that wakeups are only trivially reduced, we probably don't need to do anything special in Mojo. Does that sound OK? On Wed, Jun 11, 2014 at 8:52 PM, Darin Fisher <darin@chromium.org> wrote: > MojoWaitMany takes a timeout. It is similar to poll, select, > WaitForMultipleObjects, etc. > > -Darin > > > On Wed, Jun 11, 2014 at 8:50 PM, Mark Mentovai <mark@chromium.org> wrote: > >> Interesting. How do timers work in MessagePumpMojo then? >> >> >> On Wed, Jun 11, 2014 at 11:11 PM, Darin Fisher <darin@chromium.org> >> wrote: >> >>> Background: >>> >>> My goal is to move most of our threads to be based on MessagePumpMojo. >>> That blocks on a call to MojoWaitMany (essentially just a cvar under the >>> hood). The idea with that change is to minimize signalling latency for >>> consumers of Mojo interfaces. Otherwise, we'll have an extra PostTask to >>> deliver such signals back to the thread of the consumers. >>> >>> -Darin >>> >>> >>> On Wed, Jun 11, 2014 at 6:55 PM, Mark Mentovai <mark@chromium.org> >>> wrote: >>> >>>> The NSRunLoop shouldn’t be harmful in the renderers. It was important >>>> to get rid of the NSApplication run loop because that reached out to the >>>> system in undesirable ways, but an NSRunLoop and CFRunLoop are basically >>>> just loops. >>>> >>>> If we don’t actually use any of the features provided by >>>> NSRunLoop/CFRunLoop then I agree that we should move off of them and on to >>>> MessagePumpDefault for the renderer’s main loop, but if we’re getting >>>> something of value from them, then they shouldn’t cause any trouble. >>>> >>>> >>>> On Wed, Jun 11, 2014 at 8:05 PM, Darin Fisher <darin@chromium.org> >>>> wrote: >>>> >>>>> I'm a little nervous about this change. There is a bug open to >>>>> actually stop using NSRunLoop in the renderer process. >>>>> >>>>> See: >>>>> https://code.google.com/p/chromium/issues/detail?id=306348 >>>>> >>>>> Can we find a way to avoid this? >>>>> >>>>> -Darin >>>>> >>>>> >>>>> On Wed, Jun 11, 2014 at 3:46 PM, <commit-bot@chromium.org> wrote: >>>>> >>>>>> CQ is trying da patch. Follow status at >>>>>> https://chromium-status.appspot.com/cq/jeremy@ >>>>>> chromium.org/331513002/20001 >>>>>> >>>>>> >>>>>> https://codereview.chromium.org/331513002/ >>>>>> >>>>> >>>>> >>>> >>> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
OK, I agree. It'll be interesting to see what benefit this change has. Please CC me on the report. I'd like to follow along. Thanks! https://codereview.chromium.org/331513002/diff/20001/base/message_loop/messag... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/331513002/diff/20001/base/message_loop/messag... base/message_loop/message_loop.cc:254: return MESSAGE_PUMP_DEFAULT; I don't see the value in defining the MESSAGE_PUMP_DEFAULT macro that only gets used once. Wouldn't it be better to just put the #if defined(OS_MACOSX) bit right here? Maybe you were just following the pattern of MESSAGE_PUMP_UI. That one is similarly odd.
The CQ bit was checked by jeremy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremy@chromium.org/331513002/20001
Some notes about timer coalescing APIs on various platforms: https://docs.google.com/document/d/1cDpBIrXhjIsKCJMP-gKU8O7KLiYJHa8v4k8CPFvd9...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
Message was sent while issue was closed.
Change committed as 276808
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/336603005/ by jeremy@chromium.org. The reason for reverting is: Broke Jingle unit tests on the bots.. |
