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.

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

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.

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?

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

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

Neat. This makes things so much peppier.

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.

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?

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 alex@cendio.se and then apply this unified patch to your tree?

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

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?

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! =)

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

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.

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

Took a bit longer than expected, but here is the new patch. This zip file contains all my work, unified with the contributions from alex@cendio.se. 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.

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.

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

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.

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

I'm having problems with my build environment so I'm having rods@netscape.com try the patch on WIN32 today. I'll also line someone up to try it out on Mac today.

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. beard@netscape.com 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.

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

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.

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

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?

sfraser@netscape.com is going to try the patch on the Mac, but he may not get to it until tommorow.

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

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.

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.

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

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

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

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

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

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%. rods@netscape.com 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 - blizzard@redhat.com? If the higher authorities decide this should be M14, then so be it.

Simon wants to test rest of today, check in tomorrow. staff@mozilla.org have an interest in this getting into M13, both for timely response to outside contributor (michael.lowe@bigfoot.com, 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

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.

> 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.

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.

My work here is done. For now, anyway.

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

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

would that imply we ever had PP? :)

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

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.

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

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.

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

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

Checked in. Let me know of any problems.

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

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

please ignore, massive spam giving jrgm@netscape.com backlog of XPToolkits resolved fixed bugs to verify