Closed Bug 157144 Opened 23 years ago Closed 22 years ago

Painting gets suppressed more than it probably should

Categories

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

Product:
Core
Component:
DOM: UI Events & Focus Handling
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.2alpha

People

(Reporter: jesup, Assigned: kmcclusk)

References

(
URL
)

Details

(Keywords: perf, regression, Whiteboard: [adt2] [ETA 09/10])

Attachments

(1 file, 12 obsolete files)

Same as last patch + Doug's suggestion for sizeof(OSVERSIONINFO)
14.54 KB, patch
dougt
: review+
kinmoz
: superreview+
jesup
: approval+
Details | Diff | Splinter Review
Example: go to that url, select the Drag example, play around. Note that updates can get very delayed, even on fast machines (probably much worse on slow machines). This is a well-known issue with the event queues on various platforms (including Win32 and Linux, maybe not on Mac). Paint events get lower priority than the mouse/invalidate events, and if the mouse keeps moving enough, the paint never happens. Perhaps we should limit the amount of time that a paint can be suppressed? or flush the event queue after processing a mouse move? or coalesce mouse moves? (this might have some side effects if someone tries to do (for example) build a drawing program, but I'm not sure that's a big issue.) See also bug 142635 and bug 153083 (and probably others). Original reporter is pep.pepe@razorfish.de in a posting in n.p.m.performance
Keywords: perf
Blocks: 154568
This bug depends on #21762 and should be fixed with progression in #140789
Keywords: regression
Hardware: PC → All
Taking this bug
Assignee: joki → kmcclusk
This patch demonstrates that the paint starvation issue can be improved by forcing periodic synchronous paints. This patch is not adequate however since it will force a synchronous paint only if a reflow has been processed. It is still possible that the paints could be starved by computations that don't result in reflows. In addition, I have measured a 3.5% slowdown in jrgm's pageload tests with this patch installed. Probably the result of forcing each page to paint before firing the onload handler.
Attachment #92892 - Flags: needs-work+
Priority: -- → P1
Target Milestone: --- → mozilla1.2alpha
Blocks: 129640
Here is a snippet for msdn documentation for WIN32 which describes the issue: "With the exception of the WM_PAINT message, the system always posts messages at the end of a message queue. This ensures that a window receives its input messages in the proper first-in, first-out (FIFO) sequence. The WM_PAINT message, however, is kept in the queue and is forwarded to the window procedure only when the queue contains no other messages. Multiple WM_PAINT messages for the same window are combined into a single WM_PAINT message, consolidating all invalid parts of the client area into a single area. Combining WM_PAINT messages reduces the number of times a window must redraw the contents of its client area. "
"The WM_PAINT message, however, is kept in the queue and is forwarded to the window procedure only when the queue contains no other messages" Since we rely on the WM_PAINT events to be dispatched from the message pump we will never paint unless the msg queue is empty.
Keywords: nsbeta1
Whiteboard: [adt2 RTM] [DIGBug]
QA Contact: rakeshmishra → desale
Blocks: 117436
Blocks: 124178
Blocks: 155160
Work in progress: - Schedules a PL_Event to perform a synchronous paint whenever a widget has been invalidated. This prevents paint starvation caused by continuous scheduling of PL_Events which happens when data arrives quickly. (Examples include: Loading a local large file from disk or loading a large file from the cache). - Synchronizes the display before processing user events (Mouse and keyboard). This prevents paint starvation caused by continuous processing of a series of user-events.
Attachment #92892 - Attachment is obsolete: true
Hmm.. how will this keyboard event thing affect editor performance?
The low priority of paint events is somewhat Windows-specific, isn't it? Wouldn't it also be fixed by backing out the changes in revision 3.41 of widget/src/windows/nsAppShell.cpp, since we used to force different prioritization of paint events, but then changed to the standard Windows prioritization to improve performance (IIRC, a significant page load performance gain, perhaps close to 10%, which made Mozilla's performance roughly equivalent to that of winEmbed). (Does GTK do something similar? Could we fix it there in a similar way?) Also (mainly in reply to comment 0), paint suppression and the prioritization of paint events are two very different issues.
(w.r.t. rev 1.41 of nsAppShell.cpp, I tried reverting that change (see http://bugzilla.mozilla.org/show_bug.cgi?id=132759#c99) and in current builds I'm not seeing it as a significant difference anymore. Either I'm on crack, or something has changed to make this moot).
"The low priority of paint events is somewhat Windows-specific, isn't it?" I believe both GTK and WIN32 handle paint events the same way. They do not dispatch pending paint events until the event queue is empty. They gather up the invalidated areas and dispatch a single paint to limit the number of paint dispatches. I'm not sure about the Mac. Since Gecko is embedded in other applications which have their own message pump we can not count on any special handling of the platform specific messages. I was told that one of the reasons Hyatt removed the special event handling in nsAppShell.cpp was so that Mozilla behaved like embedded applications in regard to how event messages were processed. The embedded application has control over the event loop and we can not require them to process paint messages ahead of other events.
Attachment #94073 - Flags: needs-work+
As part of the embedding API's I see we do have a stub NS_HandleEmbeddingEvent(msg, wasHandled) which currently does nothing except return NS_OK. Perhaps we could implement this and process all of the events as a FIFO list. But we would need to restrict it to process only the events for embedded window. WIN32 does have support for processing only the events associated with a particular window and it's children. I think the fundamental problem with our event processing is that we favor particular messages over others when processing the event queue. This leads to starvation of the lower priority messages. I think we should experiment with processing the messages in the order in which they are posted.
Keywords: nsbeta1 → nsbeta1+
There are three major issues that I am seeing with event starvation on WIN32. 1. UI Lockup/Hang when attempting to moving a top-level window, resizing the top-level window, or click on native menu within an embedding app while a page is loading from the net, cache, or local disk. Mozilla completely locks up until the entire page is loaded. 2. Paint events are starved when loading a page from the cache or local disk. 3. Paint events are starved when mouse pointer is moved. This makes it difficult to implement drag and drop using JavaScript, CSS, DOM because we will never update until the pointer stops moving. The mouse move events are processed with a higher priority then the paint events so the element is not displayed until the user stops dragging. case 1. Can *not* be solved by reverted back to revision 3.41, which inverts the event priority placing the WM_APP below particular events, because once we initiate a window move,or resize Windows spins a separate event processing loop. We have no control over the event processing at this point. case 2. Can be solved by reverting to revision 3.41 as dbaron suggests. But we would also have to force embedding apps to do special processing of their event queues. case 3. Can *not* be solved by reverted to revision 3.41 because the paints are still at a lower priority than the mouse events. To solve the first two cases, I think we have to abandon the use of WM_APP events as the mechanism for PL_Event notification in plevent.c. Instead we could use a WM_TIMER event with a duration of 0. The TIMER events have the lowest priority so all system events will be processed at a higher priority which fixes both case 1 and case 2. case 3 may be solved by forcing a synchronous paint of the invalidated areas when specific user events such as mouse move, mouse down etc. are processed in nsWindow.cpp. I think case3. should be handled as a separate issue. I will spin off a new bug for it. Test case: 1. Save a large file such as (http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSFrameConstructor.cpp) locally. 2. Load the local file. 3. Try moving the window by grabbing the title bar. Mozilla freezes until the entire page is loaded. No user interaction will happen once it freezes. Try resizing the window. It freezes. Try clicking a menu item. It does not display. 4. Use mfcembed to load the local file. The page will not display until the entire page has loaded. Try clicking on a menu or moving the top-level mfcembed window while the local file is loading and the app will freeze until the entire page is loaded.
The patch i've attached changes plevent.c to use a WM_TIMER message instead of WM_APP message as the PL_Event notification mechanism. benefits: 1. Mozilla remains interactive during page loads from the net, disk, or cache. 2. Embedded applications, mfcembed, winembed remain interactive during page loads without modifying their event processing loops. 3. Mozilla and embedded applications incrementally display the page without modification of their event processing loops. 4. CPU utilization for www.netscape.com dropped from 51% to 25%. I have run jrgm's page loader tests on 750Mhz WINXP machine over a cable modem. I can *not* detect a performance degradation but my precision is only within 6%. (http://cowtools.mcom.com/page-loader/loader.pl) jrgm: can you apply the patch and run a comparison to see if page load is affected? If you apply the patch you will use WM_TIMER events which makes Mozilla responsive during page loads. if you comment out #define USE_TIMER_EVENT and recompile, it will revert back to the current behavior which uses WM_APP events.
Attachment #94073 - Attachment is obsolete: true
Comment on attachment 94661 [details] [diff] [review] Use a WM_TIMER event instead of WM_APP event for PLEvent notification These changes look good and the inital data sounds great. Lets get this checked in soon so we have lots of time this milestone to detect any problems. I haven't seen a comment reference win16 in some time! r=dougt
Attachment #94661 - Flags: review+
I'll give this a run through later today.
(note: using the updated patch, attachment 94693 [details] [diff] [review], for the tests below). I did a series of runs, first with a Buffy (current trunk) build, with and without USE_TIMER_EVENT defined. Then I repeated them for mfcEmbed standalone builds, with and without the define. On win2k, this was about 4% slower for the Buffy build mfcEmbed build. However, the "slower" builds were more responsive in e.g., I can resize and move the window while loading nsCSSFrameConstructor.cpp in the patched build, but I'm locked out of the game with the unpatched build (on both 98 and 2k). mfcEmbed, win2k, 500MHz, 12MB ------------------------------- Without patch: Avg. Median : 781 msec Average : 791 msec With patch : Avg. Median : 813 msec Average : 821 msec ----- +4% Buffy, win2k, 500MHz, 12MB ------------------------------- Without patch: Avg. Median : 870 msec Average : 874 msec With patch : Avg. Median : 905 msec Average : 909 msec ----- +4% [Un]fortunately, I ran the same tests on a win98 system. I'm really sorry to say that (apparently) something seriously goes wrong on that OS. mfcEmbed, win98SE, 400MHz, 128MB --------------------------------- Without patch: Avg. Median : 1037 msec Average : 1062 msec With patch : Avg. Median : 1226 msec Average : 1246 msec ----- +18% Buffy, win98SE, 400MHz, 128MB --------------------------------- Without patch: Avg. Median : 1132 msec Average : 1152 msec With patch : Avg. Median : 1312 msec Average : 1324 msec ----- +16% The performance degradation occurs on all the test pages.
FWIW the problem trying to be fixed here isn't a problem on os/2 because it doesn't merge paint requests and it automagicly bumps execution priority of threads having window messages waiting. I'd wish y'all check out the result of these changes on linux, mac, os/2, etc, before doing anything drastic... and the change proposed here does make drastic change in message delivery patterns, I think.
Sam: The proposed change would only affect WIN32. Linux and Mac use completely different mechanisms for the native PL_Event notification which are unaffected by the proposed change. I'm glad to hear that OS2 does not suffer from this issue. "...the change proposed here does make drastic change in message delivery patterns" Yes, it does on WIN32.
> I can resize and move the window while loading nsCSSFrameConstructor.cpp in the > patched build Cool! Can you also scroll using the scrollbar and using the keyboard? (For bug 129640.)
This is the same problem as we see in bug 132759 when Flash uses WM_USER+1 messages. To reduce the timer performance hit, would it work to use a timer for every 10th or 20th time (or something like that)?
"would it work to use a timer for every 10th or 20th time" Unfortunately no. The problem is the timer callback is not called soon enough, not that it is called too much. There seems to be a large lag between timer callbacks even though the duration is set to 0. If I set up the timer to generate WM_TIMER events instead of using the callback and I also modify the nsAppShell event processing to pull WM_TIMER's off the event queue first it is *still slow*. However, If I modify nsAppShell to process any pending PL_Events before processing any WM_ events then performance is back to normal.
Brian: Oops.. I misunderstood your last comment. Yes it would probably help performance to only use timers every n times.
How about using WM_TIMER, but with the following changes: -- in nsEventQueue::ProcessPendingEvents, after calling PL_ProcessPendingEvents the first time, check to see if there are now more pending events, and if there are, go back to the beginning and process them. Repeat until the queue is empty. -- BUT before each call to PL_ProcessPendingEvents, call ::GetQueueStatus() to see if there are any pending paint or input messages, and if there are, return immediately. For maximum responsiveness one could even call ::GetQueueStatus() inside the event processing loop in PL_ProcessPendingEvents.
roc wrote: >How about using WM_TIMER, but with the following changes: >-- in nsEventQueue::ProcessPendingEvents, after calling PL_ProcessPendingEvents >the first time, check to see if there are now more pending events, and if there >are, go back to the beginning and process them. Repeat until the queue is empty. I recall this is how plevent.c used to work, several years ago, and combined with lengthy compute-bound control flow into gecko from plevents, it led to horribly high latency and poor UI responsiveness. I'm not positive that WM_TIMER was the message type used, but I am sure the old-old plevent.c code used to loop till the queue was empty. The change troy and others made was to count its population at the top of PL_ProcessPendingEvents, and loop over only that many events. /be
> it led to horribly high latency and poor UI responsiveness Yeah, that's why I suggested using ::GetQueueStatus to leave the loop as soon as incoming input or painting is detected.
roc: I saw that, but it still seems like other uses (are there any?) of PL_PPE could suffer the change in semantics. Maybe no one else calls PL_PPE, and this is all a matter of (re-)definition. /be
There don't appear to be any other uses of PL_ProcessPendingEvents (except possibly in the BeOS nsToolkit? Whatever...) One uninterrupted run through the queue per call to PL_ProcessPendingEvents is probably necessary to make our "deferred painting" work. (Basically, when a bunch of reflows and repaints occur at once, we need to make sure that the painting happens AFTER all reflows have been processed, so we delay the invalidates by issuing custom PLEvents which then issue native invalidates. We want to finish procesing the originally-pending PLEvents before those native invalidates are serviced.) So don't try to stick a call to ::GetQueueStatus inside PL_ProcessPendingEvents itself.
"... before each call to PL_ProcessPendingEvents, call ::GetQueueStatus() to see if there are any pending paint or input messages." There is almost always a pending paint message as the result of invalidates from the first PL_ProcessPendingEvents call so we end up doing almost the same number of SetTimer's and pay the price of their dispatch latencies. We could toggle between using Timers and WM_APP events using ::GetQueueStatus to determine if there are any input messages. This would allow Mozilla to remain interactive during long page loads, but this would not solve the paint starvation issue if their isn't any user input during the page load. However, the paint starvation issue could be solved separately, by scheduling a PL_Event to do synchronous paint of any invalidated area when a nsWindow is invalidated.
I'm not positive I understand all the subtleties of PL_ProcessPendingEvents (brendan and others: slap me if I'm not making sense) but I'm okay as long as we realize that ::DispatchMessage(&msg) actually loops sucking events out of the 3 queues (unprioritized) when the user is moving/resizing the window. This of course means that posted messages are serviced by windows code (out of our control) before user events like mouse drag and keyboard when the user is moving/resizing the window.
I'm also sure I don't understand this patch well... that said, why SetTimer with elapsed=0 ? There's a maximum useful rate to process screen changes. Screen vertical refresh rate ranges 50-100Hz, that is 10-20ms, so pumping screen updates faster than this is visually ineffective. Sam
Using set timer is not about delaying the activity to some later time but about making the *new* request be serviced *after* the user input that is *already* waiting in the hardware message queue. If Microsoft had done the obivious thing of making user input on a UI based system have top priority instead of bottom priority then we would not have to be fiddling like this.
overview of what has been discussed in this bug that hopefully will clear up some questions: When a PL_PostEvent is called a WM_APP msg is Posted to the WIN32 native message queue. (If one has not been posted already). The PLEventQueue is separate from the native event queue. The PLEventQueue will often contain numerous events while there will be only one WM_APP msg posted to the native message queue. The WM_APP event is used as notification to indicate there is at at least one event and possible more events in the PLEventQueue that need to be processed. (Other platforms use different mechanism for this notification. Pipes are used on XP_UNIX) When control returns to the message processing loop which performs TranslateMessage, DispatchMessage it will process the WM_APP msg that was posted and call a routine which in turn makes a call to PL_ProcessPendingEvents which processes pending PL_Events. When each PL_Event in the PLEventQueue is processed it may call PL_PL_PostEvent which puts additional events on the PL_EventQ. The posting of the PL_Event will result in a new WM_APP event being posted to the native message queue. To prevent continuously looping over the PL_Events in the PLEventQ as the result of this additional message posts we only process the number of events that are sitting in the PL_EventQ at the time PL_ProcessPendingEvents is called. In theory this should work fine. Except for the fact that messages that are "posted" to the native WIN32 message queue using ::PostMessage are processed *before* all other messages. We overcome this somewhat in nsAppShell where the main message loop for Mozilla resides. It processes the WM_MOUSE and WM_KEY events first by peeking into the native message queue and pulling them out before the WM_APP events. But this technique completely falls apart when the user grabs the title bar of the window or tries to resize the window, because Windows spins its own event loop which will process the "posted" messages ahead of all other messages. When the posted WM_APP events are processed they often result in a new WM_APP event being inserted into the native message queue because additional PL_Events are generated. This causes a complete lockup of the UI because the message loop spends all of its time working on the posted messages, while all other events mouse move, paints etc. remain in the native message queue until the "posted" native messages stop coming in. Interestingly the 4.x code base does not have this problem even though it also uses "posted" WM_APP events. But it uses PL_Events sparingly. They are only used to indicate data has arrived from the network. In Gecko the PL_events are used for network data notification, Reflow notification, timer notification, paint invalidation. etc.,etc. My solution was to use a timer's instead of "posted" WM_APP messages.. The reason timers fix this problem is because they generate "non posted" messages. Since the native timer messages are not posted, but instead they are generated by windows internal timer code they do not starve out other "non posted" events which occur when the user moves or resizes the window. The problem with SetTimer is there seems to be a latency before the actual timer dispatch which slows down page load. Roc's proposed solution was intended to eliminate the number of times SetTimer needed to called. It involves processing more PL_Events without returning to the native message pump. He was noting that many of the calls to process the PLEvents result in additional posts to the PLEventQueue. He proposed that we continue processing the additional posted PL_Events unless there was a pending input or paint event in the native message Q. In practice the native message queue will often contain a paint event which is the result of invalidation performed while processing the PL_Events so we return immediately, and end up performing the same number SetTimer calls. If I use Roc's proposal and return only when there is an input event, performance improves but is still not as good as the when the WM_APP messages where used. It appears to be about 2% slower than use WM_APP messages on WinXP. I also tried switching dynamically between using WM_APP events and WM_TIMER events based on the user input. When user input is pending I used timers. This brings performance which matches using WM_APP messages, but the app is sluggish when dragging or resizing the window. (You are not completely "locked out" but we are not as nearly as responsive as I.E or 4.x during the long page load using this technique.) A good solution would be if we could find a way to generate a "non posted" event that we can use for notification other than using timers. Could pipes be used on WIN32 just as they are on UNIX?. Is there a way of connecting a WIN32 pipe to the GUI event processing so we can register a callback when data arrives on the pipe. If the pipe implementation on WIN32 used posted messages this would not work because we would be back to the original problem.
How about if we add a lower priority queue and test for starvation? Perhaps we could move the WM_USER, WM_APP, and PL_Event* messages to a non-OS queue that we prioritize lower than the system level keyboard/mouse queue. The basic code would look like this: set starvation timer while (1) { did_something = false if (have event other than wm_paint or wm_timer) { get next message (in natural order) (except wm_paint or wm_timer) if (wm_user or wm_app message) move to lower priority queue else service the message did_something = true if (not starving) go to start of loop } if (have lower priority message) { service lower priority messages for a while did_something = true if (not starving) go to start of loop } if (have wm_paint) { service wm_paint messages for a while did_something = true if (not starving) go to start of loop } if (have wm_timer) { service wm_timer messages for a while did_something = true if (not starving) go to start of loop } if (did_something == false) // nothing to do wait for a message/timer to arrive/fire reset starvation timer } I want to _not_ remove wm_paint messages from the OS message queue early. The OS naturally combines update regions and this minimizes repainting. We still have to handle the window move/resize case. This is the case where inside the DispatchMessage system call it internally reads the queue (ie: we cannot manage the queue). One way to deal with this is by having code in the window proc that detects wm_user and wm_app messages that have not been moved to the lower priority queue. In the window proc we can then move them to the lower priority (non OS) queue.
Brian: The patch I attached does something similar to what your proposing. It keeps metrics on event starvation before decided whether to post a WM_APP message or set a timer. The concept is that the posting allows us to send a high priority notification and setting a timer allows us to send low priority notification. If we are not staving input or paint events then we keep posting WM_APP events. If we detect starvation we use a timer event which allows any pending input and paint events to be processed. I need to do some more tuning and testing, but the patch fixes the UI lockup problem/paint starvation issue and seems to do it without affecting page load performance. I've been trying to avoid doing more special processing in the nsAppShell::Run since embedded applications have their own event loops and it may not be possible for them to modify message processing priorities.
Note: The patch I attached uses counters to indicate event starvation.(i.e number of consecutive times a WM_APP msg was posted while there was a pending input or paint) It would probably be better to compare elapsed time instead. I'm going to experiment with using elapsed times instead.
Kevin: any thoughts on what to do when a plugin like Flash is posting events and is able to use 100% of CPU?
I find I can fix the "Flash plugin uses 100% CPU" and starves out user input bug, bug 132759. define a window proc in the window proc test if input is pending: if (HIWORD(GetQueueStatus(QS_INPUT))) { defer the message by putting in on a timer with delay 0 }
Brian: Great! This approach is compatible with attachment 95604 [details] [diff] [review]. So our general solution appears to be to demote the priority of WM_APP, WM_USER events by converting them to timer events when input is starved. Should you also include paint starvation? Do the drop-down menus in Mozilla appear quickly and correctly on pages which use a plugin which generates WM_APP and WM_USER events?
> Should you also include paint starvation? Yes, I had not looked at paint yet as I've been worried about the "moz freezes the *entire* UI and requires Ctrl-Alt-Delete to break the freeze" problem. Assuming paint and timer messages come thru the window proc perhaps we can add something like this to the window proc (assuming it does not have a significant performance issue). DWORD currentTickTime = GetTickCount(); // tend the starvation timers if (msg.message == WM_PAINT) lastPaintTime = currentTickTime else if (msg.message == WM_TIMER) lastTimerTime = currentTickTime // check for starvation else if (msg.message >= WM_USER) { PRBool shouldDelay = PR_FALSE; // check for input starvation (right now!) if (HIWORD(GetQueueStatus(QS_INPUT))) shouldDelay = PR_TRUE; // check for paint/timer starvation (every now and then) else if (currentTickTime != lastTickCount) { lastTickCount = currentTickTime; if ((currentTickTime-lastPaintTime) > MAX_PAINT_STARVATION_TIME) { if (GetQueueStatus(WM_PAINT)) shouldDelay = PR_TRUE; else lastPaintTime = currentTickTime } if ((currentTickTime-lastTimerTime) > MAX_TIMER_STARVATION_TIME) { if (GetQueueStatus(WM_TIMER)) shouldDelay = PR_TRUE; else lastTimerTime = currentTickTime } } // I'm not sure if we need to test for WM_HOTKEY or if that is // handled as a posted message below WM_USER if (shouldDelay) defer the message by putting in on a timer with delay 0 }
Shanjian: if this works I believe we may no longer need the patch you just added for IME event loop problems, bug 57332.
I realized that the use of a single lastTickCount timer to limit the number of times I check probably won't work exactly like I expect if there are multiple sources calling PostMessage. else if (currentTickTime != lastTickCount) { lastTickCount = currentTickTime; if ((currentTickTime-lastPaintTime) > MAX_PAINT_STARVATION_TIME) { It probably needs to look at each message time individually. Just servicing one message and then resetting lastTickCount ignores other messages. I suspect I should just drop lastTickCount and do something like this: else if ((msg.time-lastPaintTime) > MAX_PAINT_STARVATION_TIME) { Will this same kind of issue apply to your patch? +static PRBool _md_EventIsStarved(PRBool isPending, PRUint32 ... + if (isPending) { + /* was_pending must be false so record the current time + * so the elapsed time can be computed the next time this + * function is called + */ + *lastTime = PR_IntervalToMilliseconds(PR_IntervalNow()); + *wasPending = PR_TRUE; + return(PR_FALSE); + }
The drag example in http://www.domapi.com/ seems to gone. :-( Another good url when you dont see what you are dragging is: http://www.dhtmlcentral.com Just drag around the different Windows. Cannot see them in Windows 2000 while been dragged.
I filed bug 163526 for the drag starvation issue. This bug (157144) covers only the starvation issue when loading from the cache or local file.
Whiteboard: [adt2 RTM] [DIGBug] → [adt2 RTM]
"Will this same kind of issue apply to your patch?" Brian: I don't think this is a significant issue for attachment 95717 [details] [diff] [review]. It would be more accurate to use the actual message time of the msg that was starved but the approximation of using the last time that a msg was posted with a pending paint or input msg seems to be good enough. I separate the paint and input times so their elapsed times are calculated separately. For paint events we would need to capture the time of the first invalidate. Since paints are accumulated the time that is returned in the msg structure is not the first time. AFAIK it always gives back the current tick count making it useless for calculating the elapsed time.
using attachment 96068 [details] [diff] [review] I recorded 6774 calls to PostMessage vs. 39 calls to SetTimer when running jrgm's page load tests. SetTimer was called during page load due to paint starvation. Since SetTimer is used infrequently during page load unless the user is interacting with the UI, page load times should not be affected.
> Since SetTimer is used infrequently during page load unless the user is > interacting with the UI, page load times should not be > affected. For bug 132759 my results feel very similar to this: very few calls to delay the messages except when the user is dragging/resizing the window (and with my current test patch the UI response is quite acceptable).
jrgm: Could you run your perf test again with attachment 96197 [details] [diff] [review]? Thanks! I can not detect any performance impact using the patch. In addition SetTimer is only called for approx 0.5% of the notifications when running your page load tests.
Attachment #96197 - Attachment is obsolete: true
I have completed the fix (attachment 96247 [details] [diff] [review]). It is ready to be reviewed.
I've opened bug 164084 to discuss whether we should remove the PeekMessage code. Please cc: yourself if interested.
I do not have any knowledge of PL_Events but I have been looking at MSWin event loops quite a bit recently and the timer code design and logic looks good to me. +#define PAINT_STARVATION_LIMIT 750 Its my gut level feeling that when navigating menus with a mouse a delay of over 250 milliseconds make it hard for the user. I note that this does not look for timer starvation. Do / should we care about timer starvation here?
>I note that this does not look for timer starvation. Do / should we >care about timer starvation here? Basicly, that's my problem with this fix, to the extent that 'I have a problem'. Win32 GPI is what it is, does what it does, it works the way it works. Once you take on task of 'fixing it' to behave differently in one bit of code.... then where do you stop? I raise this as a question. Don't say I know the answer.
"Its my gut level feeling that when navigating menus with a mouse a delay of over 250 milliseconds make it hard for the user." Whe navigating menus the INPUT_STARVATION_LIMIT of 50 ms is what will be triggered, So the App will remain responsive during the long page load. I have confirmed this by loading a local copy of a long file. I am able to mouse over menu items and the user response is acceptable. When the input starvation limit has been reached, a timer is used which allows all pending input AND paints will be processed before the PL_Event notification, so the effective update rate when there is a user interaction is around 50 ms not 750ms. The PAINT_STARVATION_LIMIT is triggered when the user has not interacted with the page at all during a long page load of cached or local file. Mozilla timers are implemented using the PL_Event mechanism. We will not starve out the Mozilla timers because they are already being processed as PL_Events. "Basicly, that's my problem with this fix, to the extent that 'I have a problem'. Win32 GPI is what it is, does what it does, it works the way it works" Actually this fix does *not* change the way the WIN32 GDI works. It does not attempt to fix it. It doesn't invert the order of any messages or similar nastiness, which is what is currently going on in nsAppShell by the way. In fact, we will probably be able to remove the code in nsAppShell which pulls out the mouse and key events ahead of other events once this patch goes in. It also allows embedded apps to run using unmodified msg processing loops. The WIN32 message processing does not work exactly the same way as other platforms. Thats to be expected. The Widget and Gfx modules are littered with code where we have identified native platform behavior discrepancies and make them compatible with the XP code's expectations. This is just happens to be another instance of that. This patch recognizes how the WIN32 msg queue processing works (Posted events are always processed first) and uses a mechanism for dispatching events PL_Events which is compatible with it. In Mozilla the basic model for dispatching most everything is a PL_Event. Currently we use a WIN32 GDI Mechanism which makes the notification the highest priority because it is a posted event. WIN32 GDI also provides a mechanism for providing a notification with the lowest priority which is the timer. This patch just tries to make an intelligent choice when to use one or the other. When there is user interaction we want to use the lowest priority because we want to favor interactively over page load time which means using a timer. When there isn't any user interaction we want to use the fastest mechanism which is a posted event. This is not a case where we are in the dark about what is really going on here. We know exactly why the starvation occurs. We have the microsoft documentation of how it processes posted events, paint Events and timers. With that knowledge, a solution has been crafted which takes into account microsoft's implementation and the Mozilla cross platform code expectations. Will there be other cases which we will have to address? Maybe. But the current patch solves the major UI lockup and paint starvation issue when loading a long document. I think we can build upon this solution for other starvation issues. The concept is pretty simple. We can either post high priority or low priority notification and we need to determine when to use one over the other.
What's the landing plan? If you want this in 1.2 we should get it reviewed and landed ASAP.
> When navigating menus the INPUT_STARVATION_LIMIT of 50 ms is what will be > triggered, ... so the effective update rate when there is a user interaction > is around 50 ms not 750ms. That's plenty fast enough, Since this should also be the case for my plugin patch I will go experiment with extending the paint starvation time there. > We will not starve out the Mozilla timers because they are already being > processed as PL_Events. I thought I saw the timer starvation test firing occasionally when testing my plugin patch. Are there any non-mozilla timers we need to care about?
Comment on attachment 96247 [details] [diff] [review] Improved patch: Use WM_ENTERSIZEMOVE and WM_EXITSIZEMOVE to detect when the user is moving the window 1. Maybe move or copy the comment above _md_EventIsStarved about starvation limits to where the #defines are. I was wondering what where were for and had to search for other references. 2. Remove the win16 defines. lets not kid ourselves. 3. Please be consistent with the return statement: + return(PR_TRUE); /* pending and over the limit */ + } + + return (PR_FALSE); /* pending but within the limit */ + } either a space or no space , not both. I would rather have you just drop the parenthesis. 4. You are not removing the property set by SetProp. You probably can do the same for the WindowsHookEx, but it is not as critical.
Note: I removed all of the WIN16 references in plevent.c
Attachment #96247 - Attachment is obsolete: true
Comment on attachment 96735 [details] [diff] [review] patch incorporating Doug's suggestions thanks. r=dougt Just a thought, maybe we should have a #define which disables this optimization.
Attachment #96735 - Flags: review+
Comment on attachment 96735 [details] [diff] [review] patch incorporating Doug's suggestions sr=kin@netscape.com I like doug's idea of using ifdefs, initially, so that we can turn on/off this code to see if it's contributing to any unforseen regressions.
Attachment #96735 - Flags: superreview+
Attachment #96735 - Attachment is obsolete: true
> What's the landing plan? If you want this in 1.2 we should get it reviewed > and landed ASAP. I assume roc actually meant 1.1. But I don't think we really need to force this into 1.1 at the last moment. Sorry, I didn't get some more tests in (something about some other release and all :-). I'm updating my trunk build now and should have results later tonight. (I'll also be checking win98 as well, since we previously saw a much different response to changes in event behaviour on that platform than on a NT derivative.)
I meant 1.2. There's no one anyone is going to force anything into 1.1 at this point :-). I was just wondering if you're planning to land this in 1.2. This kind of change has been very regression-prone in the past and the trunk is already quite unstable. We need to land this with plenty of time before 1.2 is due to ship to give us time to deal with any possible regressions.
*** Bug 164931 has been marked as a duplicate of this bug. ***
Blocks: 164931
(roc: sorry for misunderstanding your comment). I built the trunk optimized from this afternoon (but reverted the pref change that regressed pageload by a couple of percent [bug 30088] in my build). I applied attachment 96761 [details] [diff] [review] and tested the A and B builds on win2k and win98 win2k/500MHz/128MB Avg. Median : 919 msec Average : 929 msec Avg. Median : 923 msec Average : 936 msec -------- -------- +0.4% +0.7% win98/400MHz/128MB Avg. Median : 1181 msec Average : 1206 msec Avg. Median : 1211 msec Average : 1232 msec -------- -------- +2.5% +2.1% So, the win2k result is effectively none (I don't consider that change to be statistically significant). The win98 change does appear to be real, although it may be acceptable given that the browser should be more responsive now. It's also curious that almost all of the change on win98 occurs because of >10% slowdowns on four particular dummy pages (zdnet.com, gamespot.com, DOM Core, voodooextreme.com). I should probably test this again on win98 to be sure. Also, perhaps there is some "characteristic" of those pages that leads to this slowdown (i.e., perhaps we can identify something that can be independently fixed for those pages specifically, or win98 generally). Note: I didn't do any testing of the UI responsiveness (although I'll look at that tomorrow on win2k and win98).
jrgm: The 2.5% to 2.1% perf hit on Win98 is probably the result of those pages triggering the paint starvation limit more frequently. So they are painting more than they used to. Could you try changing following define in plevent.c? from: PAINT_STARVATION_LIMIT 750 to: PAINT_STARVATION_LIMIT 2000 And re-run your test on Win98. Thanks!
Blocks: 165039
I tried Kevin's suggestion to change the value to 2000 for PAINT_STARVATION_LIMIT and reran the test on win98. This did improve things although it did still come in about a percent slower than without the patch at all, with most of the "loss" coming in the two slowest/largest pages in the test sequence. Should I try a different setting for that define? I did try playing with the browser while loading nsCSSFrameConstructor.cpp and the browser is more responsive, both resizing and moving while the document loads. (Subjectively it seems more responsive on win2k/500MHz than on win98/400MHz, although I don't know if that is due to the speed or the OS difference).
"Should I try a different setting for that define?" 3000 or 4000 should bring it below 1%. The relationship is linear. 750 results in a 2.5% slowdown and a setting of 2000 results in 1% slowdown. 3000 should bring it to approx .6% 4000 should bring it to approx .5% I think .6% on WIN98 is a reasonable price to pay. The .6% slowdown is result of legitimate painting of the page. Part of our current page load performance is the result of starving painting for long periods of time. Setting a limit of 3 seconds on the starvation seems reasonable even if it slows the page load tests on WIN98 down by .6%.
I agree that it is reasonable to take a bit of absolute load slow down to give the user feedback that something is happening. I myself would opt for the shorter time with better feedback and slightly slower loading. It is very disconcerting to see moz "freeze up" even for 4 seconds. John: could you tell us the times for the pages that seem to be most affected?
John: and the URLs so I can get a feel of loading the pages?
There's nothing to stop us using different values for different Windows versions. We could do a run-time test and use 750 on NT systems and 3000 on Win9x systems.
Comment on attachment 97188 [details] [diff] [review] Same as the last patch + roc's suggestion to determine the paint starvation limit based on OStype. WIN9x=3000 all others=750 Can you do something like this instead: os.dwOSVersionInfoSize = sizeof(OSVERSIONINFO); This way it is more clear that you are talking about the size of the type.
Attachment #97188 - Attachment is obsolete: true
Comment on attachment 97212 [details] [diff] [review] Same as last patch + Doug's suggestion for sizeof(OSVERSIONINFO) sr=kin@netscape.com Not a big deal, but I was just wondering, since the starvation limit will never change after it is calculated by _md_GetPaintStarvationLimit(), should we be caching the value in _pl_NativeNotify to avoid the function call overhead to _md_GetPaintStarvationLimit() each time we call _md_EventIsStarved()? + if (_md_EventIsStarved( (qstatus & QS_PAINT), _md_GetPaintStarvationLimit(), or is the overhead so small it's not worth it?
Attachment #97212 - Flags: superreview+
Running jrgm's page loader tests results in just over 6000 calls to _md_GetPaintStarvationLimit(). Compared with all of the calls and processing performed to actually load the pages, reflow, render, etc. etc. This has to be insignificant.
Comment on attachment 97212 [details] [diff] [review] Same as last patch + Doug's suggestion for sizeof(OSVERSIONINFO) r=dougt
Attachment #97212 - Flags: review+
"could you tell us the times for the pages that seem to be most affected?" b b/a c c/a a ------------------------------------------------------------- www.w3.org_DOML2Core 3924 105% 3827 103% 3730 www.voodooextreme.com 4022 107% 3917 104% 3762 where a is without the patch, b is the patched build with '750', and c is the patched build with '2000' as PAINT_STARVATION_LIMIT, and these numbers generated on win98/400MHz/128MB. The URLs to "static" copies of these test pages are: http://cowtools.mcom.com/perf/content/base/www.voodooextreme.com/index.html http://cowtools.mcom.com/perf/content/base/www.w3.org_DOML2Core/index.html I agree that taking a small hit on win98 in exchange for not locking up the UI is the right choice (of course).
I checked in attachment 97212 [details] [diff] [review] If you suspect this checkin is causing a problem you can prevent it from using the timer by commenting out the following line in mozilla/xpcom/threads/plevent.c #define USE_TIMER
So is this fixed now, or are we leaving this open for more patches and/or tuning of the values?
Keywords: mozilla1.1 → mozilla1.2
Its fixed.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Nominating for 1.0 branch as improving DHTML perf would be good for embedding customers. Marking as edt1.0.2.
Keywords: edt1.0.2, mozilla1.0.2
Whiteboard: [adt2 RTM] → [adt2] [ETA 09/09]
desale: can you pls verify this as fixed on the trunk? thanks!
Whiteboard: [adt2] [ETA 09/09] → [adt2] [ETA 09/10]
Kevin, This one is marked "FIXED" on 08-30-2002. Which testcase do you think I should use to verify this Fix ? I tried the testcase provided at URL of this bug (Played around with Drag example) with 2002-09-09-08-trunk & 2002-08-15-04-trunk & I did not find any difference in painting updates (No difference in Results). If this one is Fixed on 08-30-2002, then two different trunks before & after 08-30-2002 should produce different test results. Or is it marked Fixed on basis of any other test (jrgm's performance test or something else). Please let me know which test I should use to verify this fix.
Prashant: Use the following test case: 1. Save a large file such as (http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSFrameConstructor.cpp) locally. 2. Load the local file. 3. Try moving the window by grabbing the title bar. Before this patch went in Mozilla would freeze until the entire page is loaded. No user interaction would happen once it froze. Try resizing the window. It freezes. Try clicking a menu item. It does not display. With the current trunk builds It should remain interactive. 4. Use mfcembed to load the local file. The page will not display until the entire page has loaded. Try clicking on a menu or moving the top-level mfcembed window while the local file is loading and the app will freeze until the entire page is loaded. With a current trunk build it should display after a few seconds well before the entire document has completed its load.
Yes, With testcase provided by Kevin, I can see that problem is fixed in todays trunk. I tested two trunks one 2002-09-09-08-trunk & other 2002-08-15-04-trunk from last month before fix checkin. I can see the bug is fixed. I tested on WinXP. Marking verified
Status: RESOLVED → VERIFIED
Keywords: edt1.0.2 → edt1.0.2+
Randell/Chofmann: Looks like the EDT wants this on the 1.0 branch. Can you pls provide 1.0 branch approval for this patch?
Keywords: approval
Comment on attachment 97212 [details] [diff] [review] Same as last patch + Doug's suggestion for sizeof(OSVERSIONINFO) a=rjesup@wgate.com for 1.0 branch. CHange mozilla1.0.2+ to fixed1.0.2 when checked in. Please check in ASAP for 1.0.2 tagging.
Attachment #97212 - Flags: approval+
Fix checked into the 1.0 branch.
Keywords: mozilla1.0.2+ → fixed1.0.2
desale: pls verify this as fixed for the 1.0 branch, then replace "fixed1.0.2", with "verified1.0.2". thanks!
Verified on 1.0 branch
Keywords: fixed1.0.2 → verified1.0.2
This bug was listed as OS all, but from what I gather from reading the comments, it is fixed on Windows only. If that's correct, have separate bugs been filed for Linux and Mac on this issue, as Kevin indicated he would do for bug #165039? The fix for this and bug #165039 sound like something that'll really make a huge difference in the interactive feel of Mozilla, and even if we give up one or two percent in page load time it'll definitely be worth it. Looking forward to seeing these fixes make it to Linux :-)
Filed bug 179995 to remove nsAppshell code that munges order of mouse/key events as per comment 60 here. Regarding other OS's, from a comment here: Sam: The proposed change would only affect WIN32. Linux and Mac use completely different mechanisms for the native PL_Event notification which are unaffected by the proposed change. I'm glad to hear that OS2 does not suffer from this issue. "...the change proposed here does make drastic change in message delivery patterns" Yes, it does on WIN32.
*** Bug 125260 has been marked as a duplicate of this bug. ***
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

Creator:
Created:
Updated:
Size: