Closed Bug 78611 Opened 23 years ago Closed 23 years ago

Timers need to be made threadsafe.

Categories

(Core :: XUL, defect, P1)

x86
All
defect

Tracking

()

RESOLVED FIXED
mozilla0.9.8

People

(Reporter: pavlov, Assigned: pavlov)

References

Details

Attachments

(3 files, 11 obsolete files)

60.90 KB, patch
Details | Diff | Splinter Review
34.71 KB, patch
brendan
: review+
brendan
: superreview+
Details | Diff | Splinter Review
14.11 KB, patch
Details | Diff | Splinter Review
Timers need to be made threadsafe.
Blocks: 78300
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.
Target Milestone: --- → mozilla0.9.1
Priority: -- → P4
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.
Status: NEW → ASSIGNED
Attached patch Patch to use the new timer impl (obsolete) — Splinter Review
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!)
Keywords: patch
Attached patch latest patch (obsolete) — Splinter Review
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.
Blocks: 44120
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.
Priority: P4 → P3
argh, my latest patch is missing a few files (nsITimer.h, etc..) i'll post a 
new patch tomorrow.
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Blocks: 83163
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?
Whiteboard: needs r=, sr= and a=
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.
Blocks: 64992
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.
Target Milestone: mozilla0.9.2 → mozilla1.0
Blocks: 87335
Target Milestone: mozilla1.0 → mozilla0.9.5
There are some FMW(=Free Memory Writes) in Purify logs which may cause heap
corruption in some cases. IMHO we should fix this...
Blocks: 95408
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_ ...
.
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Attached patch new timer patch (obsolete) — Splinter Review
Attachment #33793 - Attachment is obsolete: true
Attachment #33853 - Attachment is obsolete: true
Attachment #34188 - Attachment is obsolete: true
Attachment #34713 - Attachment is obsolete: true
Attachment #34721 - Attachment is obsolete: true
Attachment #35783 - Attachment is obsolete: true
Attachment #55183 - Attachment is obsolete: true
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.
Whiteboard: needs r=, sr= and a=
Priority: P3 → P1
I'm hoping to finish testing and be able to land this shortly after the tree 
opens for 0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.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
Attached patch Latest patch (obsolete) — Splinter Review
Here is the latest patch that pink has been testing with.
Attachment #55190 - Attachment is obsolete: true
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).
Attachment #60336 - Attachment is obsolete: true
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.
Attached patch New diff of xpcom/threads (obsolete) — Splinter Review
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.
Attachment #61122 - Flags: review+
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...
Attachment #61122 - Flags: needs-work+
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
Keywords: patchmozilla0.9.8
> 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
Attachment #61122 - Attachment is obsolete: true
sigh.  I guess this will have to go to 0.9.8 :(
Target Milestone: mozilla0.9.7 → mozilla0.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.
Attachment #61216 - Attachment is obsolete: true
No longer depends on: 114718
No longer depends on: 114716
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
Attachment #61325 - Flags: superreview+
Attachment #61325 - Flags: review+
Filed bug 115473 on removing timer priorities.
fixed
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → 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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: