Closed Bug 197863 Opened 21 years ago Closed 21 years ago

High CPU usage after waking from sleep because of timers

Categories

(Core :: XPCOM, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.4final

People

(Reporter: sfraser_bugs, Assigned: sfraser_bugs)

References

()

Details

(Keywords: topembed+, Whiteboard: edt_c3,edt_b3,edt_x3)

Attachments

(2 files, 3 obsolete files)

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.
Blocks: 197865
Blocks: 185287
topembed nomination, we should fix this as embeddors will see it
Keywords: topembed
Keywords: topembedtopembed+
Whiteboard: edt_c3,edt_b3,edt_x3
Attached patch patch ported to trunk (obsolete) — Splinter Review
simon - can you verify that this is the correct patch?
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?
Were the suggestions you mentioed implemented on the chimera branch?
No.
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?
back over to simon per aim conversation.
Assignee: dougt → sfraser
Status: NEW → ASSIGNED
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>
Attachment #122070 - Flags: superreview?(dougt)
Attachment #122070 - Flags: review?(brendan)
Attachment #122071 - Flags: superreview?(bryner)
Attachment #122071 - Flags: review?(pinkerton)
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.
Attachment #122070 - Attachment is obsolete: true
Attachment #122073 - Flags: superreview?(dougt)
Attachment #122073 - Flags: review?(brendan)
Attachment #122070 - Flags: superreview?(dougt)
Attachment #122070 - Flags: review?(brendan)
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.
Attachment #122071 - Flags: superreview?(bryner) → superreview+
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.
Attachment #122071 - Flags: review?(pinkerton) → review+
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.
Attachment #122073 - Flags: superreview?(dougt) → superreview+
Target Milestone: --- → mozilla1.4final
be: ping?
Keywords: nsbeta1
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
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
adt=nsbeta1-
Keywords: nsbeta1nsbeta1-
Simon/Brendan/Doug: Are the patches here obsolete ?
No, they are still needed.
What is blocking the landing of those two patches ?
I gave comments, I'll gladly sr= a revised patch.

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

new patch coming to address brendan's concerns
Attachment #122073 - Attachment is obsolete: true
new diff addressing brendan's concerns
Attachment #134541 - Flags: review?(sfraser)
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.
Attachment #134541 - Flags: superreview+
Comment on attachment 134541 [details] [diff] [review]
xpcom/threads patch (diff -pwu2)

wot bryner said. Also, don't forget the widget patch.
Attachment #134541 - Flags: review?(sfraser) → review+
landed widget and xpcom changes (with bryner's changes)
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
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
Attachment #122073 - Flags: review?(brendan)
Could this checkin have caused bug 232715 ?
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: