22979 - reimplemention of Mozilla timers on Windows and Unix

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Michael Lowe]

After looking into the causes of bug #19388, it became clear that:

   1.Timers in Mozilla could be improved by treating them more like operating
system tasks - assign a priority to each one and when they become ready,
schedule their completion routines in FIFO order within priority levels.
   2.The Windows timer implementation in Mozilla had several problems, and was
best reimplemented.
   3.Repeating timers needed to be implemented.

The attached patch addresses all these issues, and in doing so fixes at least
the following bugs (on Windows only):

    #19388 : demo 13 DHTML test is vicious - starved UI & network
    #904: No repeating timers
    #273: XP submenu open delay wrong

This patch has been tested for Windows NT (including memory leak tests), but I
need some other people to review / test it, especially to make sure it has does
not mess up other platforms. Later I will need someone to check it in.

More details regarding the timer changes are provided below:

Current problems with Mozilla timers

1. Repeating timers are not available.
2. (Windows only) Timers seem to be statically linked into each .dll using them.
3. A lot of work in Mozilla is being done off timer completion routines, and
they are currently scheduled on a purely FIFO basis. If the timer completion
routines for both the user interface and Javascript DOM are ready and are
waiting to be processed, Mozilla will choose the timer that completed first to
execute. This makes the user interface at times more unresponsive than it should
be.
4. (Windows only) On a single event loop iteration, multiple timers can
potentially fire, with each of their completion routines executed before control
is retuned to the event loop. This may cause long delays away from the event
loop, leading to user interface starvation.
5. (Windows only) Timers are currently executed at a level above user events.
This can (and does in some situations) lead to starvation of user events.

Proposed Solution

Proposed solution to point 1: Provide a parameter within the Init methods in the
nsITimer interface to allow the type of timer to be specified from one of
{NS_TYPE_ONE_SHOT, NS_TYPE_REPEATING_SLACK, NS_TYPE_REPEATING_PRECISE}. The
NS_TYPE_ONE_SHOT type provides a timer which fires once only.
The NS_TYPE_REPEATING_SLACK type provides a timer that after firing, is stopped
and not restarted until notifcation routine completes.   The specified timer
period will be at least time between when processing for last firing notifcation
completes and when the next firing occurs.  This is the preferable repeating
type for most situations. The NS_TYPE_REPEATING_PRECISE type provides a timer
which aims to have constant time between firings. The processing time for each
timer notification should not influence the timer period.   However, if the
processing for the last timer firing could not be completed until just
before the next firing occurs, then you could have two timer notification
routines being executed in quick sucession.

Point 2 is fixed by instantiating timers using a XPCOM factory.

Proposed solution to points {3, 4, 5}: Since some tasks that run off timers are
more time critical than others, assign a priority value to each timer and
execute them in FIFO order within priority levels. When a timer has reached it's
deadline, it is placed on a priority queue. While the program is idle (ie. it’s
event queue is empty), the timer completion routines are executed off the queue
in order of priority, and FIFO order within priority levels. At the end of
processing each timer, if an event has arrived in the event queue, then control
is returned to event queue processing. Timers waiting in the priority queue are
again processed when the event loop is next empty. To streamline the most time
critical timers, those timers set at the highest priority level are executed as
soon as they are ready from the main event loop, bypassing the timer priority
queue.

The Patch

The patch attached addresses all the previously mentioned issues, by extending
the nsITimer interface to allow for timer prioritisation and repeating timers,
and by reimplementing the way timers are processed in Mozilla for Windows. Some
initial work to assign priorities to the timer instances in Mozilla and make use
of repeating timers has also been provided. All efforts have been made to rovide
stub routines where necessary for the other platforms so that they will at least
compile with these changes, but I have not tested these patches on any platforms
but Windows. I now need some people to test/review this code to make sure it
works for each platform, and then someone to check it in.

Results

The changes in this patch tame demo 13 on Windows (bug #19388) - previously
after loading this demo, the Mozilla user interface would become very
unresponsive and it was almost impossible to load another page. Now demo 13 is
very much improved, and it is even possible to have demo 13 running concurrently
in multiple browser Windows without any user interface slowdown vs a single
window instance. Also, this patch fixes bug #904 for Windows by implementing
repeating timers, and bug #273 by reading and using the submenu opening delay
that theWindows registry specifies.

Comment 1

[Michael Lowe]

Attachment: Patch files (ZIP file)

Comment 2

[Michael Lowe]

This code needs review for Windows, and then Unix and Mac.  Reassign to Kevin
for Windows review.

Comment 3

[alla]

Attachment: Implementation for Gtk+

Comment 4

[alla]

Ok. I implemented this for unix too. The patch contains the diffs for
mozilla/widget/timer/src/unix/gtk against the current cvs, so it collides with
the Gtk+ part of the win32 patch. Just delete that part from the win32 patch.
(Note: The win32 patch has cr+lf linewraps, so it won't patch under unix without
conversion.)

You also need to enable repeating timers for unix to test this. Just change to:
#if defined(XP_PC) || defined(XP_UNIX)
#define REPEATING_TIMERS 1
#endif
in mozilla/widget/timer/public/nsITimer.h

It works great! The UI is responsive even when viewing test13.html.
My implementation doesn't implement NS_TYPE_REPEATING_PRECISE correctly, it
always uses NS_TYPE_REPEATING_SLACK. This was very easy to implement in Gtk+
because glib already has priorities for it's timers.

Adding pavlov for review of the Gtk+ parts.

Comment 5

[Michael Lowe]

Great work Alex!!!   Good to see it works for Unix too.   For the precise
repeating type could you create another timer as soon as the first one fires (in
nsTimerExpired) for the next timer period, then timer->FireTimeout(), and
delete/remove the old timer?

Comment 6

[alla]

Yes, I'll do that tonight. Should be fairly simple.

Comment 7

[alla]

Attachment: New Gtk+ implementation. This one does NS_TYPE_REPEATING_PRECISE too.

Comment 8

[alla]

Ok. I added a new Gtk+ implementation. This one does NS_TYPE_REPEATING_PRECISE
timers too. It replaces the former patch.

Comment 9

[Christopher Blizzard (:blizzard)]

Neat.  This makes things so much peppier.

Comment 10

[Kevin McCluskey (gone)]

I tried unzipping Michaels 2000-01-03 23:23 attachment and applied the patch
timer_diff_c.txt and I get the following errors:

S:\mozilla\widget\src\windows\nsAppShell.cpp(81) : error C2065: 'nsITimerQueue'
: undeclared identifier
S:\mozilla\widget\src\windows\nsAppShell.cpp(81) : error C2955: 'nsCOMPtr' : use
 of class template requires template argument list
        ..\..\..\dist\include\nsCOMPtr.h(595) : see declaration of 'nsCOMPtr'
S:\mozilla\widget\src\windows\nsAppShell.cpp(81) : error C2065: 'kTimerManagerCI
D' : undeclared identifier
S:\mozilla\widget\src\windows\nsAppShell.cpp(81) : error C2512: 'nsCOMPtr' : no
appropriate default constructor available
S:\mozilla\widget\src\windows\nsAppShell.cpp(81) : error C2262: 'queue' : cannot
 be destroyed
S:\mozilla\widget\src\windows\nsAppShell.cpp(105) : error C2678: binary '->' : n
o operator defined which takes a left-hand operand of type 'class nsCOMPtr' (or
there is no acceptable conversion)
S:\mozilla\widget\src\windows\nsAppShell.cpp(105) : error C2039: 'HasReadyTimers
' : is not a member of 'nsCOMPtr'
        ..\..\..\dist\include\nsCOMPtr.h(595) : see declaration of 'nsCOMPtr'
S:\mozilla\widget\src\windows\nsAppShell.cpp(105) : error C2065: 'NS_PRIORITY_LO
WEST' : undeclared identifier
S:\mozilla\widget\src\windows\nsAppShell.cpp(109) : error C2678: binary '->' : n
o operator defined which takes a left-hand operand of type 'class nsCOMPtr' (or
there is no acceptable conversion)
S:\mozilla\widget\src\windows\nsAppShell.cpp(109) : error C2039: 'FireNextReadyT
imer' : is not a member of 'nsCOMPtr'
        ..\..\..\dist\include\nsCOMPtr.h(595) : see declaration of 'nsCOMPtr'
S:\mozilla\widget\src\windows\nsAppShell.cpp(110) : error C2678: binary '->' : n
o operator defined which takes a left-hand operand of type 'class nsCOMPtr' (or
there is no acceptable conversion)
S:\mozilla\widget\src\windows\nsAppShell.cpp(110) : error C2039: 'HasReadyTimers
' : is not a member of 'nsCOMPtr'
        ..\..\..\dist\include\nsCOMPtr.h(595) : see declaration of 'nsCOMPtr'
NMAKE : fatal error U1077: 'cl' : return code '0x2'
Stop.
NMAKE : fatal error U1077: 'C:\PROGRA~1\MICROS~1\VC98\BIN\NMAKE.EXE' : return co
de '0x2'
Stop.
NMAKE : fatal error U1077: 'C:\PROGRA~1\MICROS~1\VC98\BIN\NMAKE.EXE' : return co
de '0x2'
Stop.
NMAKE : fatal error U1077: 'C:\PROGRA~1\MICROS~1\VC98\BIN\NMAKE.EXE' : return co
de '0x2'

Do I have the wrong attachment? Or do I need to do some fixup after the
attachment and patch are applied. I am trying to apply the patch to 1/11/2000
10:00am pull of mozilla.

Michael: I also tried the patch you sent me on 1/2/00 and I get the same errors.

Comment 11

[Stuart Parmenter]

this doesn't build for me either.  i get:

In file included from
../../../../../../widget/timer/src/unix/gtk/nsTimerGtk.cpp:24:
../../../../../dist/include/nsITimer.h:82: warning:
`nsITimer::Init(nsITimerCallback *, unsigned int)' was hidden
../../../../../../widget/timer/src/unix/gtk/nsTimerGtk.h:52: warning:   by
`nsTimerGtk::Init(nsITimerCallback *, unsigned int, unsigned int, unsigned int)'
../../../../../dist/include/nsITimer.h:68: warning: `nsITimer::Init(void
(*)(nsITimer *, void *), void *, unsigned int)' was hidden
../../../../../../widget/timer/src/unix/gtk/nsTimerGtk.h:52: warning:   by
`nsTimerGtk::Init(nsITimerCallback *, unsigned int, unsigned int, unsigned int)'
../../../../../../widget/timer/src/unix/gtk/nsTimerGtk.h:52:
`NS_PRIORITY_NORMAL' was not declared in this scope
../../../../../../widget/timer/src/unix/gtk/nsTimerGtk.h:52: confused by earlier
errors, bailing out


am i doing something wrong?

Comment 12

[Michael Lowe]

Kevin:  The .zip file also contains the following new files which must be
installed, as well as applying the patch:

mozilla/widget/timer/public/nsITimerQueue.h
mozilla/widget/timer/src/windows/nsIWindowsTimerMap.h
mozilla/widget/timer/src/windows/nsNewTimer.cpp
mozilla/widget/timer/src/windows/nsTimerManager.cpp
mozilla/widget/timer/src/windows/nsTimerManager.h
mozilla/widget/timer/src/windows/nsWindowsTimer.h

Did you install these?

pavlov: Did you replace the {nsTimerGtk.cpp, nsTimerGtk.h} parts of my patch
with the ones from [e-mail] and then apply this unified patch to your
tree?

Comment 13

[alla]

There was some chunks in the win32 patch that didn't apply because of some
changes. I had to apply these manually.

Comment 14

[Kevin McCluskey (gone)]

I seem to have a large number of chunks that were not applied. I have been
manually adding the missing pieces for about 1hr and I'm still not there. Some
things got applied, many others did not.

Michael: any chance you could sync with the latest code and post a new patchfile
for WIN32?

Comment 15

[Adam D. Moss]

After a lot of pain I managed to get Mozilla/GTK patched up and running,

and it's looking pretty good -- definitely an enormous improvement

with test13.



A few problems, however, which it is hard to attribute directly to

the patch.  For example, when going from the bugzilla front page

to the search page I get a [Confirm] dialog up which is grey and

Mozilla spins with 100% CPU usage, drawing nothing, until I

hit the WM close button, then things are happy again.  This is

new, but if it's not related to this patch (I do not have the

time soon to unpatch and recompile) then ignore me and the

patch is terrific!  =)

Comment 16

[Stuart Parmenter]

adam,  can you post a new patch that i can just apply and have it work? :)

Comment 17

[Adam D. Moss]

After trying for half an hour, I fear that the answer is... no!  :(



I'll try for a while longer before going to bed and attach if I succeed.

Comment 18

[Adam D. Moss]

Attachment: all-in-one (hopefully) patch for GTK testing against current CVS

Comment 19

[Michael Lowe]

I'm creating a new patch: should have it up in the next hour...

Comment 20

[Michael Lowe]

Attachment: unified patch

Comment 21

[Michael Lowe]

Took a bit longer than expected, but here is the new patch.   This zip
file contains all my work, unified with the contributions from [e-mail].
This should hopefully build and work for all platforms, though only Windows and
Gtk will have repeating prioritised timer features available.   File contains
two patch files:

timer1_diff_u.txt
timer2_diff_u.txt

both of which must be applied.   Zip file also contains some new files which
must be installed for Windows, in the following locations:

mozilla/widget/timer/public/nsITimerQueue.h
mozilla/widget/timer/src/windows/nsIWindowsTimerMap.h
mozilla/widget/timer/src/windows/nsNewTimer.cpp
mozilla/widget/timer/src/windows/nsTimerManager.cpp
mozilla/widget/timer/src/windows/nsTimerManager.h
mozilla/widget/timer/src/windows/nsWindowsTimer.h

Hopefully this stuff can be checked in soon so it does not become stale
against the Mozilla tip again.

Comment 22

[Kevin McCluskey (gone)]

I copied the files and applied the two patches on WIN32 and it worked. :)
After that, I did a clobber_all and build_all on everything.

I am getting a crash in mozilla that is not always reproducible. Usually it
crashes, If I go to demo2 then to any other demo.

It dies on line 558 in nsFrameImageLoader.cpp where it makes the following call:

 rv = view->GetViewManager(*getter_AddRefs(vm));

The view has been destroyed (view's vfptr=0xdddddddd)

nsFrameImageLoader::DamageRepairFrames(const nsRect * 0x0012fb24) line 558 + 33
bytes
nsFrameImageLoader::Notify(nsIImageRequest * 0x02738a90, nsIImage * 0x02724fa0,
nsImageNotification nsImageNotification_kPixmapUpdate, int 0, int 0, void *
0x0012fb60) line 420
ns_observer_proc(void * 0x0273cc90, long 4, void * 0x0012fbd4, void *
0x02738a90) line 97
XP_NotifyObservers(OpaqueObserverList * 0x0273ee80, long 4, void * 0x0012fbd4)
line 259 + 28 bytes
il_pixmap_update_notify(il_container_struct * 0x0273dc80) line 307 + 18 bytes
il_flush_image_data(il_container_struct * 0x0273dc80) line 215 + 9 bytes
ImgDCallbk::ImgDCBFlushImage(ImgDCallbk * const 0x0274b8d0) line 162 + 12 bytes
il_gif_write(il_container_struct * 0x0273dc80, const unsigned char * 0x02048fac,
long 0) line 1488
process_buffered_gif_input_data(gif_struct * 0x027a7360) line 669 + 16 bytes
gif_delay_time_callback(void * 0x0273dc80) line 713 + 9 bytes
timer_callback(nsITimer * 0x027a10f0, void * 0x027a23d0) line 71 + 12 bytes
nsTimer::Fire() line 183 + 17 bytes
nsTimerManager::FireNextReadyTimer(nsTimerManager * const 0x01a3fa30, unsigned
int 0) line 117
FireTimeout(HWND__ * 0x00000000, unsigned int 275, unsigned int 31139, unsigned
long 872693426) line 88
USER32! 77e7185c()
nsAppShellService::Run(nsAppShellService * const 0x00c7b780) line 466
main1(int 1, char * * 0x00c04060) line 622 + 32 bytes
main(int 1, char * * 0x00c04060) line 710 + 13 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77f1ba06()

When it crashes it is when I leave a page with images on it.

Comment 23

[Kevin McCluskey (gone)]

*** Bug 19388 has been marked as a duplicate of this bug. ***

Comment 24

[Kevin McCluskey (gone)]

Forget my 2000-01-13 13:37 comment on the crash.
I just did a clean pull minus timer changes and I still get the crash.
The new timer implementation is not the culprit.

Comment 25

[Michael Lowe]

So what is the status of this bug now?   If we get someone to review patch on
Mac, can we get this checked in?

Comment 26

[Kevin McCluskey (gone)]

I'm having problems with my build environment so I'm having [e-mail]
try the patch on WIN32 today.

I'll also line someone up to try it out on Mac today.

Comment 27

[Kevin McCluskey (gone)]

Rod is running successfully with the timer patch on WIN32 mozilla. He ran
through the standard test0-test14, chofmanns browser buster, and brought up
mail without any problems.

[e-mail] is going to try the new timer implementation on Mac. The
patch utility is not available on the Mac so he'll have to copy over the patched
files from a Linux or WIN32 box so this may take him a while.

Comment 28

[alla]

So, beard. How's it going?
It would be very cool to get this in before M13 freeze.

Comment 29

[Michael Lowe]

Note: verifying this on the Mac probably involves little more than a matter of
making sure it compiles.   All of the old timer code is still intact on that
platform, and basically only the interfaces have been extended.

I too would love to see this in before M13 freeze.

Comment 30

[Kevin McCluskey (gone)]

Moving to M14 since this is not a blocker for M13.

Comment 31

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Eeek.  We have a bunch of bugs related to DHTML animation performance, and UI
performance during such animations, and two of them are M13 for me.  It sounds
like this stuff is well-tested, assuming that beard didn't find anything really
evil on the Mac: can we get it in?

Comment 32

[Kevin McCluskey (gone)]

[e-mail] is going to try the patch on the Mac, but he may not get to
it until tommorow.

Comment 33

[Kevin McCluskey (gone)]

The patch has been verified on both WIN32 and Linux.
Reassigning to Simon for testing on the Mac.

Comment 34

[Simon Fraser [no longer active]]

OK, I'm looking at this on Mac, and I'm confused.

What files should I be expected to try to build on Mac? All the .cpp files in the
attachments to this bug are either windows or Unix specific files; I see no xp
files. I also see no implemenation for nsWindowsTimer which I can crib on Mac.

I think this is more work than the more recent entries in this bug imply. Please
advise.

Comment 35

[Michael Lowe]

All that needs to be done to verify on Mac:

1. apply the patches on a Windows or Unix box
2. copy the modified files to a Mac box, replacing the existing files
3. rebuild modified tree
4. check that Mozilla compiles, executes, and works correctly with these changes
in tree - should be possible just by running normal demos and general
observation of things things that use timers (eg. progress bar, cursor, animated
images, demo13 dhtml, etc).
5. Provide authorisation for checkin for Mac platform.

Comment 36

[Michael Lowe]

Note: to apply the patches on a Windows or Unix box (a step mentioned in last
post), take the last .ZIP file (attachment 4207 [details]), extract the files
timer1_diff_u.txt and timer2_diff_u.txt into a directory above the /mozilla
directory and run the commands:

patch < timer1_diff_u.txt
patch < timer2_diff_u.txt

Comment 37

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

*** Bug 904 has been marked as a duplicate of this bug. ***

Comment 38

[Michael Lowe]

Attachment: new .ZIP patch file

Comment 39

[Michael Lowe]

Updated patch files again to avoid conflict with checkin to nsRepeatService.cpp

Comment 40

[alla]

Btw. When this patch is checked in (and mac timers are done) nsRepeatService
should really die a violent horrible death.

Comment 41

[leger]

Per pdt, this is high risk, please consider move to M14.

Comment 42

[Michael Lowe]

This stuff has been very well tested on Windows (I have been running with it for
weeks), and I assume Gtk.   I would consider risk of problems on Windows to be
<5%.   [e-mail] ran through the top 100 sites, mailnews, demos etc on
Windows without problems.   Once we hear from sfraser that this stuff compiles
and runs on Mac, risk on this platform would be similiar.   I don't know about
Gtk, since I haven't tested it.   BTW, who can we cite as reviewer on Gtk -
[e-mail]?

If the higher authorities decide this should be M14, then so be it.

Comment 43

[Brendan Eich [:brendan]]

Simon wants to test rest of today, check in tomorrow.

[e-mail] have an interest in this getting into M13, both for timely
response to outside contributor ([e-mail], who rods now vouches
for but who lacked CVS access until recently); and for a more responsive UI.

Why does PDT consider this change high-risk?

/be

Comment 44

[Simon Fraser [no longer active]]

I diffed the changes in my tree, and it seems to build fine on Mac. On running,
the number of crashes I had was not discernably greater than normal.

I note that the new timer features (prioritization etc) are not implemented on
Mac. Since Mac does not use OS timers, but implements everything itself, someone
with a clue should implement these.

Comment 45

[Phil Peterson]

> Why does PDT consider this change high-risk?

Speaking for myself, three reasons: (1) No M13-stopper bugs rely on this. (2)
it's a big change to make when you're trying to freeze a milestone build (3) As
far as I can see, the only extensive testing it's had is on Windows.

I take on faith that these are helpful changes which will make the product
better, but now is not the time to make such changes. Early in M14 is.

Comment 46

[alla]

I agree with phil, but i think its a bit sad that it has taken so long to get
this reviewed. It could have been in the tree for several weeks now.

Comment 47

[Simon Fraser [no longer active]]

My work here is done. For now, anyway.

Comment 48

[Brendan Eich [:brendan]]

Phil, blizzard appeared to use this patch on Linux.  Blizzard, what's the word?
Pavlov may have been running with it too.

Before this goes to M14, I'd like to see some thought given to how to expedite
patch-taking.  Internal developers and those with CVS access don't have to wait
so long for similar or larger changes to land.

/be

Comment 49

[Simon Fraser [no longer active]]

I'm still concerned at the lack of a Mac implementation. If these changes go in
on Windows and Linux, and Mac is left with stubs, then we'll lose PP

Comment 50

[Stuart Parmenter]

would that imply we ever had PP? :)

Comment 51

[Brendan Eich [:brendan]]

Platform parity is good, but it's never simultaneous.  I think we're better off 
taking this now (because it's a well-tested, overdue patch), then having a Mac 
good samaritan do the Mac impl for M14.  Simon, would that good samaritan be 
you?

/be

Comment 52

[Michael Lowe]

To get repeating timers working on Mac probably involves less than a dozen lines 
- you just need to start a new timer at the beginning or end of the timer 
notification routine.   I would have already done it myself but for the lack of 
a decent Mac to test it on here.    Prioritied timers probably isn't much work 
either.

Comment 53

[Christopher Blizzard (:blizzard)]

I ran this for a while.  It seemed to be pretty painless, as near as I could
tell.

Comment 54

[chris hofmann]

brendan, jar, and I talked to several folks about this.  here is the plan.
lets get this as the first carpool in to m14 as the tree opens after
the branch.

who will have changes needed to get the carpool landed?
We may be able to do this tomorrow afternoon.

Comment 55

[Brendan Eich [:brendan]]

Michael has CVS access, so I'm passing this back to him for juggling with 
sfraser and ultimate closure after the carpool and Mac pp.

/be

Comment 56

[Adam D. Moss]

Hi folks.

I've been running with Michael and Alex's patches
on GTK for over a week now and am quite pleased with
the results.  Objectively, however, I can see the reasons
for wishing to keep this out of M13 at this point since it
simply hasn't had a chance to run free in the trunk for
general testing yet (it could have done so with some
expediency, but that's spilled milk now and M13 is
really near).  M13 is very important, and while this
patch fixes real problems and I enjoy it, I also support,
as an 'outsider', the idea of keeping nontrivial fixes
out of milestones until they've had a period in the trunk
(especially with platform parity).

I would be overjoyed to see this in the trunk as soon as
the tree re-opens.

--Adam

Comment 57

[Michael Lowe]

Checked in.   Let me know of any problems.

Comment 58

[Michael Lowe]

BTW: bug 904 is now open for sfraser to bring Mac into PP on this.

Comment 59

[chris hofmann]

1/21 afternoon verfication release builds will have this fix.

Comment 60

[Paul MacQuiddy]

please ignore, massive spam giving [e-mail] backlog of XPToolkits
resolved fixed bugs to verify