Timer priorities are pretty pointless. We don't use them in any sane way in our app nor do they make any sense. Look at bug 78611 for some comments on them.
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.0
I am not sure that I agree. I would agree that it is hard to have exact behavior of priority on all platforms, but throwing out priority hints altogether seams a bit radical.
"radical, adj. ... 2 : of or relating to the origin : FUNDAMENTAL" -- sounds good to me! Seriously, what good are priorities? Why would I use LOW vs. LOWEST or HIGH vs. HIGHEST? When in doubt, leave it out is a better design principle (a radical one, I suppose). Timer priorities just don't seem necessary, and note well: pav's new code completely ignores them. Adding more code to fire timers with the same deadline in priority order is work and code bloat that I claim we do not need. Let's cut to the root of the problem: if a timer client wants to order two timer callbacks that have the same delay, I claim the client should be using one timer, not two. Someone show me a counterexample quick. /be
I've never seen 2 timers end up with the same time, so comparing their priorities would be a waste of time.
Since our timer impl has a low timer resolution, I thought that it would make sense to have and ordered list as we can not enforce different clients of the thread code to share a timer callback.
Priority doesn't make up for resolution. No one says "I want this timer to run in 14.9 msec and that one in 14.8 msec, so I'll set both at (what I somehow know to be the rounded delay of) 14 ms and give the 14.8 one HIGHEST priority, while I give the 14.9 one merely HIGH priority" -- or LOW and LOWEST, or whatever. This just does not happen, because (a) clients don't know the resolution; (b) clients that need to order timers have other issues we should dig into; (c) clients should try to minimize pending timers in any event (see my comments in bug 78611 about a priority queue being needed if there are more than a dozen or so timers pending). Pavlov's gdb sessions confirmed that timers seem never to share a deadline, but instrumentation output to that effect would be more convincing. Pav, can you whip some metering code up? Like, how often a deadline was shared, by how many timers maximum and average. /be
Brendan, I do not care about what mozilla is doing with timers. I am concerned about the limit cases where two clients of this thread code share the same time expiration because of our course resolution and one needs to fire prior to the other. Just tell me it aint going to be supported and I will shutup. Although, I am interested in what our resolution is. brendan mentioned ~15 msec above, I would be shocked this code consistantly produced that. Pav, any idea what the finest resolution would be, say on windows?
dougt, I think you have to overcome my point (a) first -- clients just don't know the resolution, and in fact, two timers that seem widely separated (100s of ms) could fire at the same time, worst case. A client cannot assign priorities to break this tie, in general (there aren't enough well-known priorities). IOW, I'm arguing that any client that thinks it knows what it is doing with timer priorities is broken. Pav's new code has mean+standard-deviation measurement for timer skew. Pavlov, how about some fresh numbers? Or can we easily turn it on and measure for ourselves? Give us the easy way. /be
from running with NSPR_LOG_MODULES set to nsTimerImpl:5, i'd say on windows, you're best bet is around 15ms: 1484[1c37398]: Timer thread woke up 5ms from when it was supposed to 0[3c4da8]: [this=3c25248] time between Fire() and Process(): 0ms 0[3c4da8]: [this=3c25248] expected delay time 530ms 0[3c4da8]: [this=3c25248] actual delay time 545ms 0[3c4da8]: [this=3c25248] ------- 0[3c4da8]: [this=3c25248] delta 15ms 0[3c4da8]: [this=3c25248] Took 25ms to fire process timer callback
Duh, I'm babbling: timers with different deadlines should be sorted in time order, so priority is not an issue. I was thinking of the point dougt made, that we may have ms resolution, but we have big bad latency problems due to Gecko hogging the CPU from the event loop and other threads. What I *meant* to say was that two timers with nominally the same deadline can run far apart, if the main thread runs between their firings and hogs the CPU (rjesup worries that we should avoid this case by not waiting on the condvar when firing timeouts with the same deadline). Anyway, sorry for going off half-cocked. Here's another question: if I add two timers with the same delay, and they both share the same deadline (the same PR_IntervalNow() result is used to compute their deadlines), are they inserted in the order that their delays were set (or that they were created)? They're fired in whatever order they were put into the mTimers array. Looking at the new code, AddTimerInternal uses aTimer->mTimeout < timer->mTimeout (where timer is looping over mTimers from least to greatest index), so we're cool. Timer add order is preserved. I still don't see the need for priority. But more people need to say, cuz they may have meant something that doesn't work even with priority, or that is a legitimate requirement we should address somehow (maybe not with priority). Cc'ing blizzard, dbaron, and waterson. /be
The priority should be of the threads processing the timer wakeups, not of the timers themselves. All timers with the "same" time should be processed at once, and without letting one of the threads unblocked by the timer firing run until all threads have been notified. I.e. don't unblock even with NO_WAIT on the CondVar inbetween firings for the same time; fire them all and then when we release the CPU let thread priorities handle who gets to run. If something is high priority, have it run on a high-priority thread. The only issue with what I said is that most things run on the main thread (and it blocks for long periods doing layout/etc), so order of queuing events to the thread becomes important - or the events must be queued with priorities on the events. Again, no need for priorities on the timers, if there are priorities on insertion to the event queue (or on removal). If there isn't (FIFO), then priorities on timers could have an impact - but I think the solution is to prioritize the event queue instead. IMHO.
Will this ease overal latency problems?
Since currently the priority of a timer is completely ignored (except when setting mPriority to aPriority), fixing this bug can't cause a perf increase. Unless, of course, I'm missing something here... but nsTimerImpl.cpp only sets mPriority, never reads it.
OK, so my last comment wasn't quite correct. Idle Timers do use the priority (they are enabled on windows only): Only highest priority timers fire immediately, others fire only when the system is idle. What's the problem with all timers firing immediately?
ok. pavlov said "don't ask". so I'll just change nsITimer::Init's argument aPriority to a PRBool and fix the callers. taking bug
Assignee: pavlov → cbiesinger
Status: ASSIGNED → NEW
this bug is simple about cleaning out the unused and pointless timer priorities. it won't have any impact on anything else.
Comment on attachment 86129 [details] [diff] [review] patch i'm not sure that those methods gain anything at all by being const. You won't ever have a const nsISupports object since you wouldn't be able to addref/release, so while technically correct, the const's may be a waste of thought. I don't really care one way or another though. r=pavlov
Attachment #86129 - Flags: review+
Comment on attachment 86129 [details] [diff] [review] patch email@example.com, this should go in as soon as the 1.1beta trunk opens. /be
Attachment #86129 - Flags: superreview+
moving 1.1a bugs to 1.1beta since they won't get in 1.1a anyway
Target Milestone: mozilla1.1alpha → mozilla1.1beta
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
For the record, the first of the two changes to nsGlobalWindow.cpp in this patch was wrong, it made all our JS timeouts, except consequently fired interval timeouts, be non-idle timers. See bug 153083.
You need to log in before you can comment on or make changes to this bug.