Closed Bug 164931 Opened 22 years ago Closed 22 years ago

[DHTML]PLEvents should use a timer for notification on WIN32 instead of WM_APP events when documents are not loading

Categories

(Core :: DOM: UI Events & Focus Handling, defect, P1)

x86
Windows XP
defect

Tracking

()

VERIFIED FIXED
mozilla1.2beta

People

(Reporter: kmcclusk, Assigned: kmcclusk)

References

()

Details

(Keywords: perf, Whiteboard: [FIXED ON THE TRUNK] [adt2] [ETA Needed])

Attachments

(1 file, 5 obsolete files)

After the fix for bug 157144 lands we need to investigate using timers instead of posted WM_APP events for PLEvent notification when documents are not loading. Using timers should provide the following benefits: 1. Eliminates paint starvation when running DHTML animation's by allowing all pending paints to be processed before the next Mozilla timer fires. 2. Reduced CPU utilization on pages with animation. (I have seen a 50% reduction in CPU utilization on netscape.com (which contains a news ticker)
*** This bug has been marked as a duplicate of 157144 ***
Status: NEW → RESOLVED
Closed: 22 years ago
Keywords: nsbeta1+
Priority: -- → P1
Resolution: --- → DUPLICATE
Target Milestone: --- → mozilla1.2beta
Not a dup. This bug is dependent on bug 157144
Status: RESOLVED → REOPENED
Depends on: 157144
Resolution: DUPLICATE → ---
Need to modify plevent.c to include the following changes static PRBool _md_MovingWindow = PR_FALSE; static PRInt32 _md_WindowCount = 0; static PRBool _md_DocumentLoading = PR_FALSE; <== Add this _md_EventReceiverProc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam) #endif { <== Add the following if (XPCOM_WIN32_MSG_DOCUMENT_STATUS == uMsg) { if (wParam == XPCOM_WIN32_MSG_DOCUMENT_START) { _md_DocumentLoading++; return TRUE; } if (wParam == XPCOM_WIN32_MSG_DOCUMENT_STOP) { _md_DocumentLoading--; return TRUE; } } static PRStatus _pl_NativeNotify(PLEventQueue* self) { ... if ((_md_MovingWindow) || (! _md_DocumentLoading)) { <== Add this
Attachment #96881 - Attachment is obsolete: true
With the patch installed CPU utilization for: www.netscape.com 20-27% (down from 41%) http://info.dws.de/dws/homepage.nsf/doc/home 63%-85% (down from 100%)
Blocks: 21762
Attachment #96909 - Attachment is obsolete: true
With attachment (id=97858) and a release build on WinXP. 750 Mhz machine. CPU usage varies between 1%-10% without the patch the CPU is 21%-25%
Previous comment was for www.netscape.com On http://info.dws.de/dws/homepage.nsf/doc/home CPU varies from 41%-70% without the patch CPU usage varies between 55%-86%
Keywords: mozilla1.2
Keywords: perf
Blocks: 163154
The patch solves a major event issue on WIN32. Currently, there is nothing which prevents Mozilla timers and reflow from starving paints and other events after a page is loaded. Mozilla timers set through JavaScript are implemented as a separate thread which posts a PL_Event when the time delay has expired. The PL_Event implementation for WIN32 posts native WM_APP events for notification. The posted events are processed *before* all other events including paints. The processing of the posted WM_APP event can result in the posting of additional WM_APP events. This will cause paint starvation if the interval between timer callbacks falls below a threshold which is the combination of the speed of the machine and complexity of processing in the callback. The fix is two add a hint to the PL_Event dispatch mechanism to indicate that it should not use posted WM_APP events. The hint is honored only on WIN32. The hint is passed indicating it should use the fasted dispatch possible when any document is loading. After all documents have loaded a hint is passed indicating that a mechanism which doesn't starve native events should be used. The hint call includes a delay which indicates how long to wait before using the native timer implementation after all documents are loaded. The delay was necessary to *not* impact page load times. Our page load performance is currently dependent on starving the paint and other events and quickly proceeding to the next page after the page has loaded. Thats why the delay is there. If we quickly move to a new page it will continue to use posted WM_APP events without any intervening use of a native timer.
With this patch applied, Flash animations that can use 100% the CPU can only use about 80% CPU and they appear to aminimate a bit slower. What about putting PLEvents on a timer only when there are input/paint/timer events in the queue?
Blocks: 100%CPU
"What about putting PLEvents on a timer only when there are input/paint/timer events in the queue? " I don't think there is a significant benefit to posting WM_APP events for plevent notification after a page is loaded. The only reason we don't always use native timers for plevent notification is that we want to starve paint events during page load. Since we don't want to starve paint events or other events after the page is loaded we might as well use timers all of the time. Always using timers after page load also keeps the logic a little simplier. I also think that always using timers for plevent notification after the page is loaded will help eliminate other problems since it makes Mozilla behave like typical window applications in that animations and other caculations which require periodic callbacks are run off of timers rather than posted WM_APP events.
Note: I have been running for 2 days with the patch installed on a trunk build as dogfood and I have not noticed any problems. I've run through a large number of pages with DHTML animations and I haven't noticed any performance degredation.
Whiteboard: Waiting for r/sr
is this ready for review/super-review?
yes. I asked dougt for review and kin for super-review.
rick, brendan, major plevent/docshell love coming from Kevin. Care to review?
Comment on attachment 99105 [details] [diff] [review] Improved fix - uses logic in nsDocShell.cpp to determine when page load as starts and stops and delays the resumption of using timers to prevent page load degredation I'm willing to sr=kin this if we get an r= from someone familiar with this area ... dougt, brendan, rick? Just some little formatting and comment nit picks. ==== Remove the extra "native" word: + * this hint tells the native native dispatch code which to favor. ==== Missing a period after "starvation", capitalize 'D' in "delay indicates" to match convention used in previous paragraph. + * The default is to prevent event starvation + * delay indicates how low long in milliseconds to wait before preventing + * event starvation after events have been allowed to be starved. + */ ==== Do we need to modify the comment above to be a little more explicit in saying that the |starvationDelay| arg is only used when |favorPerformanceOverEventStarvation| is PR_FALSE, and it is basically the amount of time we wait before the PR_FALSE actually takes effect? Maybe it's just me, but the way I read the comment above, before looking at the implementation, it kind of sounded like the delay arg described some period of time we would allow events to starve before we actually processed events, and that it was an ongoing (process, starve, process, ...) thing, when really it's a one time delay, since we never update _md_SwitchTime unless |PL_FavorPerformanceHint()| is called again with a PR_FALSE. ==== Put a return after the |gNumberOfDocumentsLoading| line so that is more readable: +// Number of documents currently loading +static PRInt32 gNumberOfDocumentsLoading = 0; +// Hint for native dispatch of plevents on how long to delay after +// all documents have loaded in milliseconds before favoring normal +// native event dispatch priorites over performance ==== The decrement can actually be done as |if (--gNumberOfDocumentsLoading == 0)| but it's up to you: + gNumberOfDocumentsLoading--; + if (gNumberOfDocumentsLoading == 0) { ==== Same deal here with the increment: + gNumberOfDocumentsLoading++; + if (gNumberOfDocumentsLoading == 1) { ==== FYI, I asked Kevin via email about what the possibility was of us getting stuck in "favor performance" mode was, in scenarios where the user hit the stop button in the middle of a load etc. He said that was one of the things he tested and that things worked fine. He also said that on the worst case, if it were stuck in WM_APP dispatch, that this would be the same as what we are currently doing on both the trunk and branch.
Comment on attachment 99105 [details] [diff] [review] Improved fix - uses logic in nsDocShell.cpp to determine when page load as starts and stops and delays the resumption of using timers to prevent page load degredation r=rpotts@netscape.com
Attachment #99105 - Flags: review+
Comment on attachment 99105 [details] [diff] [review] Improved fix - uses logic in nsDocShell.cpp to determine when page load as starts and stops and delays the resumption of using timers to prevent page load degredation sr=kin@netscape.com With the formatting/commenting issues addressed.
Attachment #99105 - Flags: superreview+
Attachment #99105 - Attachment is obsolete: true
I checked in attachment 99763 [details] [diff] [review] to the trunk. Note: Before checking it in. I analyzed the performance characteristics of posting WM_APP messages vs. using native timers on WIN32. I created a WIN32 testbed application that could either post WM_APP messages or set timers and I measured how long it took to process 100, 1000, and 10000 messages. The major difference between the two is that timers are limited to a minimum 10ms timer delay which produces a 100frames per second refresh rate, while Posted WM_APP events are limited only by the CPU's processing power. This has some important consequences for DHTML test cases which attempt to measure performance based on timer delays which fall below the 10ms threshold. This is true of the "sliding an element 100px to the right, setTimeout set to 0ms between steps" test case at http://www.umsu.de/jsperf/index2.php In this test case it seems to be a measure of performance. But it actually measures the maximum frame rate. The patch I just checked in slows this example down by 60% on a 750Mhz machine running WinXP. Thats because our maximum frame rate is now 100frames per second. Before the frame rate was unbounded. This DHTML test case does not seem to be a good test case in general. When running Opera6 it gave the best number by far. But when you watched it animate it only rendered the text 2-3 times (Almost all of the painting was starved) while IE and Mozilla render it smoothly but posted much slower overall times because they actually did some *real* processing in response to the timer callback. In my testbed I found that when a realistic amount of computation was performed in the callback for both the timer and posted WM_APP code paths they had the same performance. So for real world DHTML examples the 100fps maximum rate is really never approached and the difference between the timers and DHTML disappears. In general 100fps is far faster than what is needed to achieve smooth animation anyway.
Fixed
Status: REOPENED → RESOLVED
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
rakeshmishra: can you pls verify this as fixed on today's trunk? thanks!
Whiteboard: Waiting for r/sr → [FIXED ON THE TRUNK] [adt2] [ETA Needed]
Verified on build 2002-09-20-08-trunk
Status: RESOLVED → VERIFIED
No longer blocks: 21762
Blocks: 21762
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: