197863 - High CPU usage after waking from sleep because of timers

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Simon Fraser [no longer active]]

On Mac OS X, the timer code gets really confused when the machine sleeps. On
wake,  Mozilla/Camino hog the CPU for minutes, while processing timers that
should have fired during the time that the machine was asleep.

This was fixed on the chimera branch in bug 152528. See the URL for changes. We
need to migrate this to the trunk.

Comment 1

[saari (gone)]

topembed nomination, we should fix this as embeddors will see it

Comment 2

[Doug Turner (:dougt)]

Attachment: patch ported to trunk

simon - can you verify that this is the correct patch?

Comment 3

[Simon Fraser [no longer active]]

It looks OK, but it would be nice to avoid that sleep() hack. Also, maybe we
should put the SleepQ code somewhere central, and just fire an observer
notification on sleep/wake, that the timer code can listen for?

Comment 4

[Doug Turner (:dougt)]

Were the suggestions you mentioed implemented on the chimera branch?

Comment 5

[Simon Fraser [no longer active]]

No.

Comment 6

[Doug Turner (:dougt)]

I am relying on the mac developers on this patch - is it what we want for the
trunk?  Do we want to hold off for simon's suggestions?

Comment 7

[Doug Turner (:dougt)]

back over to simon per aim conversation.

Comment 8

[Simon Fraser [no longer active]]

Attachment: Patch for xpcom/threads, uses nsIObserver to get sleep/wake notifications

Comment 9

[Simon Fraser [no longer active]]

Attachment: widget/src changes; share code between cocoa/carbon, use IOKit for sleep/wake

This patch is large because I've moved some code around, to share code between
nsToolkit for Cocoa and Carbon.

I made a new base class, nsToolkitBase, and moved the shared code into it,
including NS_CreateToolkitInstance. Carbon and Cocoa each have small derived
classes, which just handle the toolkit-specific stuff.

The important parts for this bug are
nsToolkitBase::RegisterForSleepWakeNotifcations() and the
ToolkitSleepWakeCallback(), whic use IOKit functions to regiser a run loop
source that gets system power notifications. Docs are at
<http://developer.apple.com/techpubs/macosx/Darwin/General/IOKitFundamentals/PowerMgmt/chapter_10_section_8.html>

Comment 10

[Simon Fraser [no longer active]]

Attachment: xpcom/threads patch again, bogus includes removed

What this patch does is to make the TimerThread object observer sleep/wake
notifications (which, for Mac, will be posted over in widget code).

On sleep, we set a flag in the timer thread to say we're asleep, and don't fire
any timers during that time. We just take 1/10th second naps until wake time.
Since the process is probably suspended during sleep anyway, this is just a way
to keep the loop idling, without actually firing any timers. And we don't want
to fire timers while asleep, because they may not get processed by the main
thread, and could pile up.

On wake, we go through the extant timers, and reset their delay; this adjusts
their fire time to account for the time that we were sleeping. We also clear
the 'adjustment' settings so that the timer thread will recalibrate, which is
necessary to prevent it thinking that the firing of timers is delayed by the
sleep duration (and thus trying to "catch up"), and to take into account any
changes in machine load after wake.

Comment 11

[Brian Ryner (not reading)]

Comment on attachment 122071 [details] [diff] [review]
widget/src changes; share code between cocoa/carbon, use IOKit for sleep/wake

>@@ -68,24 +67,16 @@
> 
> struct PRThread;
> 
>-class nsToolkit : public nsIToolkit
>+class nsToolkit : public nsToolkitBase
> {
>-
> public:
>-  nsToolkit();
>+                nsToolkit();
>   virtual				~nsToolkit();

What exactly are you doing to the indenting here?

>Index: mac/nsToolkitBase.cpp
>===================================================================
>RCS file: mac/nsToolkitBase.cpp
>diff -N mac/nsToolkitBase.cpp
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ mac/nsToolkitBase.cpp	30 Apr 2003 03:00:48 -0000
>+static void ToolkitSleepWakeCallback(void *refCon, io_service_t service, natural_t messageType, void * messageArgument)
>+{
>+  switch (messageType)
>+  {
>+    case kIOMessageSystemWillSleep:
>+      // Handle demand sleep (such as sleep caused by running out of
>+      // batteries, closing the lid of a laptop, or selecting
>+      // sleep from the Apple menu.
>+      nsToolkitBase::PostSleepWakeNotification("sleep_notification");
>+      ::IOAllowPowerChange(gRootPort, (long)messageArgument);
>+      break;
>+
>+    case kIOMessageCanSystemSleep:
>+      // In this case, the computer has been idle for several minutes
>+      // and will sleep soon so you must either allow or cancel
>+      // this notification. Important: if you donÕt respond, there will

Some sort of non-ASCII apostrophe character in "don't"?

sr=bryner with those fixed or addressed.

Comment 12

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 122071 [details] [diff] [review]
widget/src changes; share code between cocoa/carbon, use IOKit for sleep/wake

+nsToolkitBase::PostSleepWakeNotification(const char* aNotification)
+{
+  nsCOMPtr<nsIObserverService> observerService =
do_GetService("@mozilla.org/observer-service;1");

should we cache the observer service? doesn't seem like a huge deal either way.

r=pink.

Comment 13

[Doug Turner (:dougt)]

Comment on attachment 122073 [details] [diff] [review]
xpcom/threads patch again, bogus includes removed

Since your explictly calling RemoveObserver, TimerThread does not have to be a
nsSupportsWeakReference?

you have some printf's left in this patch.

Should you do process timers in DoAfterSleep?  What about sarvation of these
event?

brendan must have the final say on this patch as he is the most familar with
this code now.

Comment 14

[Simon Fraser [no longer active]]

be: ping?

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 122073 [details] [diff] [review]
xpcom/threads patch again, bogus includes removed

Sorry, lost track of the sr request!

I applied and diff -w'd in order to see what was happening without a lot of
noise.	A diff -pwu attachment for reviewing always wins for changes such as
this (the TimerThread::Run reindentation).

Non-nit: eliminate suspendTimerFiring and just use an else-if in
TimerThread::Run.

Nits below.

>@@ -90,17 +99,27 @@
> 
>   // We hold on to mThread to keep the thread alive.
>   rv = NS_NewThread(getter_AddRefs(mThread),
>                     NS_STATIC_CAST(nsIRunnable*, this),
>                     0,
>                     PR_JOINABLE_THREAD,
>                     PR_PRIORITY_NORMAL,
>                     PR_GLOBAL_THREAD);
>+  if (NS_FAILED(rv))
>+    return rv;
> 
>+

Nit: No need for two blank lines here.

>-class TimerThread : public nsIRunnable
>+class TimerThread : public nsSupportsWeakReference,
>+                    public nsIRunnable,
>+                    public nsIObserver
> {
> public:
>   TimerThread();
>   virtual ~TimerThread();
> 
>   NS_DECL_ISUPPORTS
>   NS_DECL_NSIRUNNABLE
>-
>+  NS_DECL_NSIOBSERVER
>+  
>   nsresult Init();
>   nsresult Shutdown();
> 
>   nsresult AddTimer(nsTimerImpl *aTimer);
>   nsresult TimerDelayChanged(nsTimerImpl *aTimer);
>   nsresult RemoveTimer(nsTimerImpl *aTimer);
> 
> #define FILTER_DURATION         1e3     /* one second */
> #define FILTER_FEEDBACK_MAX     100     /* 1/10th of a second */
> 
>   void UpdateFilter(PRUint32 aDelay, PRIntervalTime aTimeout,
>                     PRIntervalTime aNow);
> 
>   // For use by nsTimerImpl::Fire()
>   nsCOMPtr<nsIEventQueueService> mEventQueueService;
> 
>+  void     DoBeforeSleep();
>+  void     DoAfterSleep();

Nit: these method declarators don't need extra indentation -- they don't line
up with anything else (unless they're supposed to line up with the nsCOMPtr
template param?).

/be

Comment 16

[Brendan Eich [:brendan]]

Using an if-else-if to eliminate suspendTimerFiring allows setting waitFor once
only per iteration of the Run loop, too.  It does mean declaring waitFor without
an initializer, but it's a p.o.d. scalar variable -- no runtime or code space
cost is sunk by the uninitialized declaration.

/be

Comment 17

[Samir Gehani]

adt=nsbeta1-

Comment 18

[Vidar Haarr (not reading bugmail)]

Simon/Brendan/Doug: Are the patches here obsolete ?

Comment 19

[Simon Fraser [no longer active]]

No, they are still needed.

Comment 20

[Ludovic Hirlimann [:Usul]]

What is blocking the landing of those two patches ?

Comment 21

[Brendan Eich [:brendan]]

I gave comments, I'll gladly sr= a revised patch.

/be

Comment 22

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 122073 [details] [diff] [review]
xpcom/threads patch again, bogus includes removed

new patch coming to address brendan's concerns

Comment 23

[Mike Pinkerton (not reading bugmail)]

Attachment: xpcom/threads patch (diff -pwu2)

new diff addressing brendan's concerns

Comment 24

[Brian Ryner (not reading)]

Comment on attachment 134541 [details] [diff] [review]
xpcom/threads patch (diff -pwu2)

>@@ -96,4 +105,13 @@ nsresult TimerThread::Init()
>                     PR_PRIORITY_NORMAL,
>                     PR_GLOBAL_THREAD);
>+  if (NS_FAILED(rv))
>+    return rv;
>+
>+  nsCOMPtr<nsIObserverService> observerService = do_GetService("@mozilla.org/observer-service;1");
>+  if (NS_FAILED(rv))
>+    return rv;

rv didn't change since the last time you checked it.

>@@ -352,3 +376,39 @@ PRBool TimerThread::RemoveTimerInternal(
>   NS_RELEASE(aTimer);
>   return PR_TRUE;
>+}
>+
>+void TimerThread::DoBeforeSleep()
>+{
>+  printf("Timers prepare for sleep\n");

probably don't want to check in that printf.

sr=bryner with those fixed.

Comment 25

[Simon Fraser [no longer active]]

Comment on attachment 134541 [details] [diff] [review]
xpcom/threads patch (diff -pwu2)

wot bryner said. Also, don't forget the widget patch.

Comment 26

[Mike Pinkerton (not reading bugmail)]

landed widget and xpcom changes (with bryner's changes)

Comment 27

[Brendan Eich [:brendan]]

Comment on attachment 134541 [details] [diff] [review]
xpcom/threads patch (diff -pwu2)

>@@ -186,4 +204,11 @@ NS_IMETHODIMP TimerThread::Run()
> 
>   while (!mShutdown) {
>+    PRIntervalTime waitFor;
>+    PRBool suspendTimerFiring = PR_FALSE;

Please remove the useless suspendTimerFiring.  Thanks,

/be

Comment 28

[Jo Hermans]

Could this checkin have caused bug 232715 ?

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This patch introduced some threadsafety problems, in particular, bug 279852. 
The observer service probably shouldn't be used on a non-main thread at all.