Closed Bug 115473 Opened 23 years ago Closed 22 years ago

Remove timer priorities

Categories

(Core :: XUL, defect, P3)

Product:
Core
Component:
XUL
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.1beta

People

(Reporter: pavlov, Assigned: Biesinger)

Details

Attachments

(1 file)

patch
37.96 KB, patch
pavlov
: review+
brendan
: superreview+
Details | Diff | Splinter Review 
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.
accepting
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.
Keywords: perf
Target Milestone: mozilla1.0 → mozilla1.1
Will this ease overal latency problems?
Keywords: mozilla1.1
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.
Keywords: perf
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.
Attached patch patch — — Splinter Review 
pavlov, can you review?
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

sr=brendan@mozilla.org, 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
checked in
Status: NEW → RESOLVED
Closed: 22 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.

Attachment

General

Creator:
Created:
Updated:
Size: