Timers need to be made threadsafe.

On unix, this will require me to remove the use of gtk_timeout_* in the code and fire plevents on the current thread's event queue. This should be fairly straight forward. don't know about mac or windows yet... I expect I will have to do something similar.

You'll need to spin up an event queue for each thread that wants to dispatch timers. From http://msdn.microsoft.com/library/psdk/winui/timers_0euq.htm: When you specify a TimerProc callback function, the default window procedure calls the callback function when it processes WM_TIMER. Therefore, you need to dispatch messages in the calling thread, even when you use TimerProc instead of processing WM_TIMER.

I've put together an XP timer implementation. I'll post patches soon.

One additional change I've made since I made this patch was in TimerThread::Run (), where it does: nsTimer *theTimer = NS_STATIC_CAST(nsTimer *, mTimers[0]); I addref 'theTimer' and release it after calling fire (kungFuDeathGrip!)

cc'ing adamlock. I believe the only reason we have NS_DoIdleEmbeddingStuff() is for windows timer handling, which this patch erradicates. Perhaps we can get rid of the function altogether? Before landing this we need to ensure that win/mfcEmbed can handle META refresh w/ this patch. I love seeing all those static lib linkages go away; hurray! pavlov, Does this patch invalidate 64992?

If timers can be piggybacked onto native windows events then yes, NS_DoIdleEmbeddingStuff and bug 44120 can be done away with. This would of course require the solution to work for every platform. We should also put big warnings inside nsAppShell::Run telling people not to put stuff in there because embedders implement their own message loops.

this should fix both 64992 and 44120... I havn't tested it since I don't have a build here that I can apply my patch to, but I am pretty sure it will fix them.. low priority timers will get put into the event queue, and get processed eventually, not only when things become idle.

ok, i want to get this checked in this weekend. I'm testing on linux right now to verify that it works.

ok, i've tested and verified on linux. need mac testing.

I can land this and leave the Mac using the old timer code until someone can test this.

hmm, i'm hitting a few monitor deadlocks in various places... I need to investigate this before getting the r= and sr=.

Get rid on the monitor. I think that you can just use the lock you have. Use a convar for notification. Also, do you think that the thread you spawn should be PR_PRIORITY_LOW?

On Mac, we fire all timers on the UI thread. If you plan to dispatch a PLEvent each time a timer fires, won't this add undue latency to timers, and increase the amount of PLEvent noise (with possible scarey event processing implications)?

Don't use monitors. Why did you, btw? I'm curious to know so we can prevent future abusage. /be

Why are we spending any scarce 0.9.1 cycles on this bug? /be

simon and/or saari: if you can test this patch (and remove widget/timer from the mac project(s)) that would be great.

argh, my latest patch is missing a few files (nsITimer.h, etc..) i'll post a new patch tomorrow.

i should move the timer thread creation into xpcom init so that it gets started earlier. Can I get someone to r=, sr= and a= this patch?

pav, here are some comments, after briefly looking at the patch... 1) Are you sure that the reference counting of the nsTimers is correct? It looks like the reference count is off by one in nsTimerThread::Run()... Presumably, when a timer is added to the mTimers list its refcount=1. It looks like calling GetNextTimer() causes refcount=2. However, there is only one NS_IF_RELEASE() in Run()... 2) You might also add comments to AddTimerInternal(...) and RemoveTimerInternal(...) indicating that a precondition is that mLock is acquired... An Assert would be even better :-) 3) In the nsITimer.h interface, why are you forcing interface methods to return 'void' rather than 'nsresult'? It seems like it would be much cleaner (and scriptable) to just return nsresult... 4) In the same vein, couldn't nsITimer.h just be an idl file where 'delay', 'priority' , 'type' and 'closure' are attributes? Granted, you couldn't have default parameters, but those are evil anyways :-) 5) Same questions about nsITimerCallback.h... Why are you forcing a 'void' return type... And why not idl..

I'll look at the refcounting stuff. nsITimer and nsITimerCallback are from the old world and I havn't changed them, only moved them. Perhaps a new timer interface would be good (i think i even have one laying around...), but it would require changing all the timer callsites, which maybe can done in round 2.

Not really. Just keep some the old stuff around with the magic |noscript| and |notxpcom| attributes. See nsIAtom.idl.

can we get some r/sr/a action here? dougt? rpotts? waterson? mueller?

well, given that they don't yet work on mac, let's not jump the gun.

So, I'm going to make a big uninformed pointy-haired comment. Can we put this stuff on a branch and see if the fancy features that it will support (e.g., putting image decoding on its own thread) acutally make a difference? I am currently of the semi-informed opinion that we have _way too much_ threading in the app, as every profile I've run of short web pages is dominated by lock contention.

Waterson, which locks are you seeing contention for? Maybe we've threaded the wrong things... Ideally, you don't need to lock much at all to decode images on a thread. Image decoding is one of those few things that you can spit data at and it can spit it back out without much synchronization needed, unlike, oh, say, database access.

I'm going to push this off to mozilla 1.0 since nothing really important is dependant on it... if we need it sooner, its here.

There are some FMW(=Free Memory Writes) in Purify logs which may cause heap corruption in some cases. IMHO we should fix this...

gisburn: what purify logs, and what code is doing the FMWs? What do they have to do with this bug? /be

brendan: See bug 83163 ("FMW: Free memory write when repeating timer is released from its callback") ... Full logs on demand... they are _huge_ ...

.

Ok, latest patch is attached. This patch includes: - new files: TimerThread.[cpp,h] and nsTimerImpl.[cpp,h] - windows makefile changes. Since timers on UNIX are a component, there shouldn't be very many build changes required to make this patch work. Unlike the previous patches, which didn't work very well on Mac, this one should work fine. I will need some help getting the mac build changes ready for this patch.

I'm hoping to finish testing and be able to land this shortly after the tree opens for 0.9.7

applied patch on mac, the following is wrong: - url bar doesn't load urls - autocomplete doesn't show - animated images don't animate - throbber barely animates during page load and then a crash submitting a form: #0 0x006d7740 in 0x6d7740 #1 0x006ad338 in nsTimerImpl::Process(void) #2 0x006ad5cc in handleMyEvent(MyEventType *) #3 0x0065e558 in PL_HandleEvent #4 0x0065e304 in PL_ProcessPendingEvents #5 0x005e53e8 in nsEventQueueImpl::ProcessPendingEvents(void) #6 0x04f732a0 in nsMacNSPREventQueueHandler::ProcessPLEventQueue(void) #7 0x04f72c90 in nsMacNSPREventQueueHandler::RepeatAction(EventRecord const &) #8 0x02803be8 in Repeater::DoRepeaters(EventRecord const &) #9 0x04f8b9fc in nsMacMessagePump::DispatchEvent(int, EventRecord *) #10 0x04f8b568 in nsMacMessagePump::DoMessagePump(void) #11 0x04f8ad18 in nsAppShell::Run(void) #12 0x02a3c984 in nsAppShellService::Run(void) #13 0x004e0858 in main1(int, char **, nsISupports *) #14 0x004e2db8 in main

Here is the latest patch that pink has been testing with.

This patch has been tested on all 3 platforms and is known to work. This patch doesn't have the unix makefiles changes, but has everything else (cept for the mac xpcom project changes).

we need to do perf tests on all 3 platforms before this lands. can you get opt builds to QA?

i ran my opt carbon build with the new timers on os9 vs the 12/5 osx build on os9 and it was within error: - with patch: 1807ms - 12/5 bits: 1816ms mac is looking good. i'm pulling to the trunk to match the 12/5 bits exactly, but i don't expect any difference, my tree is only a day or so old.

This is just xpcom/threads with a few minor things dougt wanted.

Comment on attachment 61122 [details] [diff] [review] New diff of xpcom/threads it looks okay. I would like this to bake on the trunk longer than a day before puttin it into a release, but pav can discuss that with the sr'er.

We may want this patch in 0.9.7. 2001-12-09-08-trunk has some weired timer issues (crash on start which depends on the order how components are registered) on Unix/Linux and this patch definately fixes them all.

There's no way that this is going into 0.9.7.

Christopher Blizzard: > There's no way that this is going into 0.9.7. Then we are in trouble. We may have timer problems during XPCOM registration and during printing. pav's patch fixes them...

Comment on attachment 61122 [details] [diff] [review] New diff of xpcom/threads I'm in favor of this patch getting in and getting tested on the trunk, but I see no reason to rush for 0.9.7. Do respond and fix these points, but don't waste time robbing from true 0.9.7 bugs on your list! 0. You have changed the licenses on nsITimer.h and nsITimerCallback.h to NPL from the NPL/GPL/LGPL tri-license, but (a) you don't have copyright, and (b) this is against mozilla.org and Netscape policy. Very bad, must fix! 1. Please use the MPL/GPL/LGPL tri-license boilerplate in entirely-new and copyright-Netscape files -- don't make me get your manager on your case! 2. Spacey (vertically -- why a newline between each?) #includes in TimerThread.cpp. 3. Why is mWaiting a PRInt32, yet PR_TRUE and PR_FALSE are its only values? 4. mProcessing init in TimerThread ctor should be a member call-style init, not an assignment. 5. Brace style mismatch: while (mProcessing) { 6. GetNextTimer() does its own locking, causing twice the acquire/release overhead in TimerThread::Run's while (mProcessing) loop. For what benefit is this cost exacted? 7. TimerThread::GetNextTimer depends on nsVoidArray::ElementAt returning null for an out of bounds index. Don't do that -- rjesup is changing ElementAt (called by nsVoidArray::operator[](PRInt32 aIndex)) to be inline and not bounds-checking. You'll have to test mTimers.Count() yourself. 8. TimerThread::GetNextTimer should use >=, not >, when comparing itIsNow to timer->mTimeout. 9. TimerThread::GetNextTimer and its caller, TimerThread::Run, are using raw NS_ADDREF and NS_RELEASE. Worth switching to nsCOMPtr, especially in light of point 6? 10. Insertion sort is ok if there are only a dozen or fewer timers oustanding, especially if the new timers tend to be inserted near the front. But why wonder? Please add some instrumentation that captures the maximum length of the mTimers array, and the averate insertion-point index. With it, we can maybe someday know that we should switch to a priority queue (heap). 11. NS_TIMER_CONTRACTID should not be defined in nsITimer.h. It seems to be unused at present -- can it be removed? 12. Timers are not thread-safe, in spite of their being NS_IMPL_THREADSAFE_* and the use of TimerThread. You should assert in nsTimerImpl::nsTimerImpl that the current thread is the main thread. Otherwise, at least the code to bootstrap gThread is thread-unsafe and leaky in the face of races. 13. Can you educate me on the ownership model of nsITimerCallback? The timer holds a weak ref (via the union arm mCallback.i), so what keeps the impl of the nsITimerCallback interface alive as long as the timer is alive? 14. s/effect/affect in nsTimerImpl::SetType's XXX comment (which should also be wrapped better to respect column 80, or 100, or whatever your maximum width might be here!). Also, file a bug on this XXX comment's topic and cite the bug # in the comment. 15. In the DEBUG_TIMERS #ifdef in nsTimerImpl::Process, d should be a double, not a PRUint32, or the (d * d) expression may overflow. 16. Now that I think of it, why isn't nsTimerThread::RemoveTimerInternal an inline method? 17. Nit: too many parens around mCallback.c and mCallback.i here: + if (mCallbackType == CALLBACK_TYPE_FUNC) { + (*(mCallback.c))(this, mClosure); + } else if (mCallbackType == CALLBACK_TYPE_INTERFACE) { + (mCallback.i)->Notify(this); 18. Nit: no need for typedef struct MyEventType { ... } MyEventType in C++, just say struct MyEventType { ... }. 19. Nit: indentation of underhanging args is off here: + // initialize + PL_InitEvent((PLEvent*)event, this, + (PLHandleEventProc)handleMyEvent, + (PLDestroyEventProc)destroyMyEvent); 20. Since you pass this as the owner of the PLEvent, why not eliminate the mTimer extension and use PL_GetEventOwner(event) to go from PLEvent* event to nsTimerImpl* timer? You'll still need to NS_ADDREF and NS_RELEASE, of course. But you'll save a word per PLEvent, and why malloc that word if you don't really need to? 21. So the *only* effect of priority is whether the TimerThread posts the event synchronously or asynchronously? That says priority is broken, or you need to rename it as synchonous -- which seems useless. Did you think that the PostSynchronousEvent method on nsIEventQueue caused the event to be put at the front of the queue? I find no code in plevent.c to do that. I really think we need to do away with priority, or else make it real: make sure that, for timers ready to fire, we fire them from highest to lowest priority. 22. You no longer need to hack around NS_DECL_LOG crap: +#if defined(PR_LOGGING) +// stupid NS_DECL_LOG crap +#ifdef PRLogModuleInfo +#undef PRLogModuleInfo +#endif +#ifdef PR_NewLogModule +#undef PR_NewLogModule +#endif /be

http://lxr.mozilla.org/mozilla/search?string=NS_PRIORITY makes me think we should just do away with priority -- it's an overdesign artifact, a useless (timers are unlikely to fire at the same time but with different priorities, and if they need a total firing order, they'll have to DIY) and meaningless degree of freedom for most users -- if not for all users. In case #6 and 9 weren't clear, I mean "why not do away with GetNextTimer, inline expanding its guts, minimizing locking, and using nsCOMPtr appropriately?" I'm unable to spend more time sr'ing this bug right now, because it is *so* not a 0.9.7 priority without a crash/topcrash/other-equally-compelling reason to land now, rather than when the 0.9.8 trunk opens. I'm not happy about how much time I've spent reviewing it already, or arguing about its not being a 0.9.7 bug at this stage of the patch, and given its symptoms. /be

> 0. You have changed the licenses on nsITimer.h and nsITimerCallback.h to NPL > from the NPL/GPL/LGPL tri-license, but (a) you don't have copyright, and > (b) this is against mozilla.org and Netscape policy. Very bad, must fix! > > 1. Please use the MPL/GPL/LGPL tri-license boilerplate in entirely-new and > copyright-Netscape files -- don't make me get your manager on your case! > whaah. done. > 2. Spacey (vertically -- why a newline between each?) #includes in > TimerThread.cpp. boy, arn't we picky? > 3. Why is mWaiting a PRInt32, yet PR_TRUE and PR_FALSE are its only values? I made it a PRPackedBool along with mProcessing > 4. mProcessing init in TimerThread ctor should be a member call-style init, > not an assignment. ok, sure. > 5. Brace style mismatch: > while (mProcessing) > { see answer to #2 :-) > 6. GetNextTimer() does its own locking, causing twice the acquire/release > overhead in TimerThread::Run's while (mProcessing) loop. For what benefit > is this cost exacted? It has to lock. You can't lock while Fire() is being called. I've gotten rid of GetNextTimer() all together and moved the code directly into the processing loop > 7. TimerThread::GetNextTimer depends on nsVoidArray::ElementAt returning null > for an out of bounds index. Don't do that -- rjesup is changing ElementAt > (called by nsVoidArray::operator[](PRInt32 aIndex)) to be inline and not > bounds-checking. You'll have to test mTimers.Count() yourself. true, fixed. see the answer to #6 also. > 8. TimerThread::GetNextTimer should use >=, not >, when comparing itIsNow to > timer->mTimeout. ok > 9. TimerThread::GetNextTimer and its caller, TimerThread::Run, are using raw > NS_ADDREF and NS_RELEASE. Worth switching to nsCOMPtr, especially in light > of point 6? No.. then i'm having to do static casts to nsTimerImpl all over the place.. far uglier imho. > 10. Insertion sort is ok if there are only a dozen or fewer timers oustanding, > especially if the new timers tend to be inserted near the front. But why > wonder? Please add some instrumentation that captures the maximum length > of the mTimers array, and the averate insertion-point index. With it, we > can maybe someday know that we should switch to a priority queue (heap). We rarely have more than 5-10 timers running. > 11. NS_TIMER_CONTRACTID should not be defined in nsITimer.h. It seems to be > unused at present -- can it be removed? It is used by nsXPCOMInit for component registration. > 12. Timers are not thread-safe, in spite of their being NS_IMPL_THREADSAFE_* > and the use of TimerThread. You should assert in nsTimerImpl::nsTimerImpl > that the current thread is the main thread. Otherwise, at least the code > to bootstrap gThread is thread-unsafe and leaky in the face of races. Timers are threadsafe, with, as you pointed out, potentially the first few timers firing in a potential race to create the thread. I'll talk to you about this once we solve all the other points. > 13. Can you educate me on the ownership model of nsITimerCallback? The timer > holds a weak ref (via the union arm mCallback.i), so what keeps the impl > of the nsITimerCallback interface alive as long as the timer is alive? thats up to the consumer. either use a static method or be smart. > 14. s/effect/affect in nsTimerImpl::SetType's XXX comment (which should also > be wrapped better to respect column 80, or 100, or whatever your maximum > width might be here!). Also, file a bug on this XXX comment's topic and > cite the bug # in the comment. ok, i'll file a bug on it > 15. In the DEBUG_TIMERS #ifdef in nsTimerImpl::Process, d should be a double, > not a PRUint32, or the (d * d) expression may overflow. ok. > 16. Now that I think of it, why isn't nsTimerThread::RemoveTimerInternal an > inline method? er, uh, it is. > 17. Nit: too many parens around mCallback.c and mCallback.i here: > > + if (mCallbackType == CALLBACK_TYPE_FUNC) { > + (*(mCallback.c))(this, mClosure); > + } else if (mCallbackType == CALLBACK_TYPE_INTERFACE) { > + (mCallback.i)->Notify(this); if you say so. > 18. Nit: no need for typedef struct MyEventType { ... } MyEventType in C++, > just say struct MyEventType { ... }. ok > 19. Nit: indentation of underhanging args is off here: > > + // initialize > + PL_InitEvent((PLEvent*)event, this, > + (PLHandleEventProc)handleMyEvent, > + (PLDestroyEventProc)destroyMyEvent); fun fun :) > 20. Since you pass this as the owner of the PLEvent, why not eliminate the > mTimer extension and use PL_GetEventOwner(event) to go from PLEvent* event > to nsTimerImpl* timer? You'll still need to NS_ADDREF and NS_RELEASE, of > course. But you'll save a word per PLEvent, and why malloc that word if > you don't really need to? good catch. done > 21. So the *only* effect of priority is whether the TimerThread posts the event > synchronously or asynchronously? That says priority is broken, or you need > to rename it as synchonous -- which seems useless. Did you think that the > PostSynchronousEvent method on nsIEventQueue caused the event to be put at > the front of the queue? I find no code in plevent.c to do that. > > I really think we need to do away with priority, or else make it real: > make sure that, for timers ready to fire, we fire them from highest to > lowest priority. timer priorities never really worked as one would hope. on windows, low priority timers never fired unless the app was entirely idle. timers also never seem to be close enough together that they would have multiple timers ready to fire at the same time. Perhaps we should get rid of them, but I believe that is an exercise for another day. as for synchronously vs async, you appear to be right. perhaps i should put together a patch to plevent that allows prepending of events to the queues.. fortunatly, this doesn't seem to effect anything. i've changed it to always post async events. > 22. You no longer need to hack around NS_DECL_LOG crap: > > +#if defined(PR_LOGGING) > +// stupid NS_DECL_LOG crap > +#ifdef PRLogModuleInfo > +#undef PRLogModuleInfo > +#endif > +#ifdef PR_NewLogModule > +#undef PR_NewLogModule > +#endif sweeeeeet

sigh. I guess this will have to go to 0.9.8 :(

Re: 6, I got carried away writing up the cost complaint, yeah. The thing is, if there is a timer to fire, you'll have to unlock and relock as you say; if there is no timer (how often do we find no timer?), then you can keep the lock until the wait-condvar. >>9. TimerThread::GetNextTimer and its caller, TimerThread::Run, are using raw >>NS_ADDREF and NS_RELEASE. Worth switching to nsCOMPtr, especially in light >>of point 6? >No.. then i'm having to do static casts to nsTimerImpl all over the place.. >far uglier imho. So use the concrete type (nsTimerImpl) as the nsCOMPtr<> template parameter -- that's legit if the concrete class singly inherits from nsISupports. >We rarely have more than 5-10 timers running. I believe you, that's cool for nsVoidArray -- but how did you measure that? Worth keeping that code around, or did you just peek in a debugger? Re: NS_TIMER_CONTRACTID, it's still better to keep implementation details out of interface definitions, and you can always spell the string literal out in nsXPCOMInit.cpp (it will then occur exactly once in the source, unless you have another patch that adds uses of this contract-id). Why add a contract-id that's not used, anyway? >Timers are threadsafe, with, as you pointed out, potentially the first few >timers firing in a potential race to create the thread. I'll talk to you >about this once we solve all the other points. Use PR_CallOnce? >> 16. Now that I think of it, why isn't nsTimerThread::RemoveTimerInternal an >> inline method? >er, uh, it is. No, despite the inline in front of the method declaration in the class, it's not defined in the .h file. Inline methods should be defined (not just declared) in their class. So, file a sequel bug to get rid of priority; don't mess with plevent.c. New patch? How are those true 0.9.7 bugs going? :-) Don't rush this, it's not worth it, but I'm still up at 2:23am and feeling perky again. /be

I found that there is typically 5-10 timers by breaking in the debugger at random times. This number could go up on pages with insane numbers of animated images or DHTML, but even then, most of those have very short timeouts. NS_TIMER_CONTRACTID is used by nsXPComInit.cpp by one of its macros for component registration. See my earlier patch which includes the diffs to that file. I've moved the define into nsTimerImpl.h so that it isn't in nsITimer.h. I believe this covers all of the problems you had with the earlier patches.

changes to widget and xpcom projects for mac.

Comment on attachment 61325 [details] [diff] [review] Newer patch to address brendan's problems What, no BEGIN and END LICENSE BLOCK noise? :-) Set gThread to nsnull after releasing it in static nsTimerImpl::Shutdown. Please file a bug on removing timer priority and cite it in the nsITimer.h file. You have at least one too many double(...) casts -- the outer one is unnecessary, all that is required is one of the inner (* operand) casts, but both looks nicer (symmetry). Still too many parens around (mCallback.i)-> and (mCallback.c), but if it makes you happy.... Fix the failure to null gThread, and remove the outer double cast, file that bug on removing timer priority and cite it in the code, and sr=brendan@mozilla.org. The latest patch doesn't include Makefile.in, but I'm rs'ing all build fu changes related to this code patch. /be

Filed bug 115473 on removing timer priorities.

fixed

how sweet it is; thanks pav!

dito. - Thanks pav! :-)

Minor perf nits with the patch (note: these really are minor): We can avoid an extra call to PR_IntervalNow() in the case of more than one timer being ready at the same time (by checking the value we grabbed on the first call for >= timeout before calling it again). This may not happen very often. I have a code snippet if you think you want it. Do we need to call PR_WaitCondVar() if we're not going to wait? It does release and re-acquire the condition var, and can cause a reschedule. Again, this only applies in the case of multiple firings in one wakeup. (This might not be too unusual if the timers are slow to execute - such as, perhaps, anims?) Question: Do anims do all their rendering from the main timer callback (I'd guess no from what I've seen; I'm guessing they queue events for the main thread)? If they do run on the timer callback then this is an issue for responsiveness to timer events. Did you instrument how long timers take to run?

Can we incorporate jesup's suggestions? Regarding comment #56 could this cause the regression at bug 117436?

bug 220959 seems to indicate a problem with the timer threads A talkback for one such crash is: http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB59352Q

WD: please let this bug rest in peace. The tackback you cite at bug 220959 comment 10 has nothing to do with XPCOM timer implementation details, AFAICT, and everything to do with main-thread ref-counting logic bugs. /be