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

Status: VERIFIED FIXED

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

Description

[Kevin McCluskey (gone)]

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)

Comment 1

[Kevin McCluskey (gone)]

*** This bug has been marked as a duplicate of 157144 ***

Comment 2

[Kevin McCluskey (gone)]

Not a dup. This bug is dependent on bug 157144 

Comment 3

[Kevin McCluskey (gone)]

Attachment: Change to document loader to notify internal plevent window of document state change

Comment 4

[Kevin McCluskey (gone)]

Attachment: Change to plevent.h to support changes to the document loader

Comment 5

[Kevin McCluskey (gone)]

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
   

Comment 6

[Kevin McCluskey (gone)]

Attachment: Cleaner fix. Note: the changes to plevent.c within the patch must be applied by hand

Comment 7

[Kevin McCluskey (gone)]

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%)

Comment 8

[Kevin McCluskey (gone)]

Attachment: improved fix which can be applied to the trunk

Comment 9

[Kevin McCluskey (gone)]

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%

Comment 10

[Kevin McCluskey (gone)]

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%

Comment 11

[Kevin McCluskey (gone)]

Attachment: 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

Comment 12

[Kevin McCluskey (gone)]

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. 

Comment 13

[kill this account]

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? 

Comment 14

[Kevin McCluskey (gone)]

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

Comment 15

[Kevin McCluskey (gone)]

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.

Comment 16

[kill this account]

is this ready for review/super-review?

Comment 17

[Kevin McCluskey (gone)]

yes. I asked dougt for review and kin for super-review.

Comment 18

[Doug Turner (:dougt)]

rick, brendan, major plevent/docshell love coming from Kevin.  Care to review?

Comment 19

[Markus Hübner]

Some examples for testing:

http://www.umsu.de/jsperf/index2.php 
http://www.world-direct.com/mozilla/dhtml/fish2.htm
http://3dhtml.netzministerium.de/examples/heart3d.html
http://3dhtml.netzministerium.de/examples/solarsystem3d.html
http://3dhtml.netzministerium.de/examples/molecule3d.html
http://3dhtml.netzministerium.de/examples/beziercube3d.html
http://3dhtml.netzministerium.de/examples/openinghexcube3d.html
http://audi.vw-audi.es/ttroadster/audittroadster.html
http://pascal.dynamic-core.net/greym/archives/00000016.php
http://xlat.assembler.org/
http://alladyn.art.pl/gandalf/MozillaTests

Comment 20

[kinmoz]

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 21

[rpotts (gone)]

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=[e-mail]

Comment 22

[kinmoz]

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=[e-mail]

With the formatting/commenting issues addressed.

Comment 23

[Kevin McCluskey (gone)]

Attachment: patch incorporating Kin's suggestions

Comment 24

[Kevin McCluskey (gone)]

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.  

Comment 25

[Kevin McCluskey (gone)]

Fixed

Comment 26

[Jaime Rodriguez, Jr.]

rakeshmishra: can you pls verify this as fixed on today's trunk? thanks!

Comment 27

[Rakesh Mishra]

Verified on build 2002-09-20-08-trunk