Closed
Bug 78611
Opened 24 years ago
Closed 23 years ago
Timers need to be made threadsafe.
Categories
(Core :: XUL, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla0.9.8
People
(Reporter: pavlov, Assigned: pavlov)
References
Details
Attachments
(3 files, 11 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
brendan
:
review+
brendan
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
Timers need to be made threadsafe.
Assignee | ||
Comment 1•24 years ago
|
||
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.
Assignee | ||
Updated•24 years ago
|
Target Milestone: --- → mozilla0.9.1
Assignee | ||
Updated•24 years ago
|
Priority: -- → P4
Comment 2•24 years ago
|
||
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.
Assignee | ||
Comment 3•24 years ago
|
||
I've put together an XP timer implementation. I'll post patches soon.
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•24 years ago
|
||
Assignee | ||
Comment 5•24 years ago
|
||
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!)
Assignee | ||
Comment 6•24 years ago
|
||
Comment 7•24 years ago
|
||
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
Assignee | ||
Comment 9•24 years ago
|
||
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.
Assignee | ||
Comment 10•24 years ago
|
||
ok, i want to get this checked in this weekend. I'm testing on linux right now
to verify that it works.
Assignee | ||
Comment 11•24 years ago
|
||
ok, i've tested and verified on linux. need mac testing.
Assignee | ||
Comment 12•24 years ago
|
||
I can land this and leave the Mac using the old timer code until someone can
test this.
Assignee | ||
Comment 13•24 years ago
|
||
Assignee | ||
Comment 14•24 years ago
|
||
hmm, i'm hitting a few monitor deadlocks in various places... I need to
investigate this before getting the r= and sr=.
Comment 15•23 years ago
|
||
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?
Comment 16•23 years ago
|
||
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)?
Comment 17•23 years ago
|
||
Don't use monitors. Why did you, btw? I'm curious to know so we can prevent
future abusage.
/be
Comment 18•23 years ago
|
||
Why are we spending any scarce 0.9.1 cycles on this bug?
/be
Assignee | ||
Comment 19•23 years ago
|
||
Assignee | ||
Comment 20•23 years ago
|
||
simon and/or saari: if you can test this patch (and remove widget/timer from the
mac project(s)) that would be great.
Assignee | ||
Comment 21•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Priority: P4 → P3
Assignee | ||
Comment 22•23 years ago
|
||
argh, my latest patch is missing a few files (nsITimer.h, etc..) i'll post a
new patch tomorrow.
Assignee | ||
Updated•23 years ago
|
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Assignee | ||
Comment 23•23 years ago
|
||
Assignee | ||
Comment 24•23 years ago
|
||
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=
Comment 25•23 years ago
|
||
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..
Assignee | ||
Comment 26•23 years ago
|
||
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.
Comment 27•23 years ago
|
||
Not really. Just keep some the old stuff around with the magic |noscript| and
|notxpcom| attributes. See nsIAtom.idl.
Comment 28•23 years ago
|
||
can we get some r/sr/a action here? dougt? rpotts? waterson? mueller?
Comment 29•23 years ago
|
||
well, given that they don't yet work on mac, let's not jump the gun.
Comment 30•23 years ago
|
||
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.
Comment 31•23 years ago
|
||
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.
Assignee | ||
Comment 32•23 years ago
|
||
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
Assignee | ||
Updated•23 years ago
|
Target Milestone: mozilla1.0 → mozilla0.9.5
Comment 33•23 years ago
|
||
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
Comment 34•23 years ago
|
||
gisburn: what purify logs, and what code is doing the FMWs? What do they have
to do with this bug?
/be
Comment 35•23 years ago
|
||
brendan:
See bug 83163 ("FMW: Free memory write when repeating timer is released from its
callback") ...
Full logs on demand... they are _huge_ ...
Assignee | ||
Comment 37•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Attachment #33793 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #33853 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #34188 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #34713 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #34721 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #35783 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #55183 -
Attachment is obsolete: true
Assignee | ||
Comment 38•23 years ago
|
||
Assignee | ||
Comment 39•23 years ago
|
||
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=
Assignee | ||
Updated•23 years ago
|
Priority: P3 → P1
Assignee | ||
Comment 40•23 years ago
|
||
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
Comment 41•23 years ago
|
||
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
Assignee | ||
Comment 42•23 years ago
|
||
Here is the latest patch that pink has been testing with.
Assignee | ||
Updated•23 years ago
|
Attachment #55190 -
Attachment is obsolete: true
Assignee | ||
Comment 43•23 years ago
|
||
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
Comment 44•23 years ago
|
||
we need to do perf tests on all 3 platforms before this lands. can you get opt
builds to QA?
Comment 45•23 years ago
|
||
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.
Assignee | ||
Comment 46•23 years ago
|
||
This is just xpcom/threads with a few minor things dougt wanted.
Comment 47•23 years ago
|
||
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+
Comment 48•23 years ago
|
||
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.
Comment 49•23 years ago
|
||
There's no way that this is going into 0.9.7.
Comment 50•23 years ago
|
||
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...
Updated•23 years ago
|
Attachment #61122 -
Flags: needs-work+
Comment 51•23 years ago
|
||
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
Comment 52•23 years ago
|
||
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: patch → mozilla0.9.8
Assignee | ||
Comment 53•23 years ago
|
||
> 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
Assignee | ||
Comment 54•23 years ago
|
||
sigh. I guess this will have to go to 0.9.8 :(
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Comment 55•23 years ago
|
||
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
Assignee | ||
Comment 56•23 years ago
|
||
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
Comment 57•23 years ago
|
||
changes to widget and xpcom projects for mac.
Comment 58•23 years ago
|
||
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+
Assignee | ||
Comment 59•23 years ago
|
||
Filed bug 115473 on removing timer priorities.
Assignee | ||
Comment 60•23 years ago
|
||
fixed
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 61•23 years ago
|
||
how sweet it is; thanks pav!
Comment 62•23 years ago
|
||
dito. - Thanks pav! :-)
Comment 63•23 years ago
|
||
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?
Comment 64•22 years ago
|
||
Can we incorporate jesup's suggestions?
Regarding comment #56 could this cause the regression at bug 117436?
Comment 65•20 years ago
|
||
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
Comment 66•20 years ago
|
||
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.
Description
•