44678 - PLEvent Event Handling inconsistencies

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P5)

Description

[Doug Turner (:dougt)]

When I create a native event Q with PL_CreateEventQueue on windows, events will 
be processed without any client having calling PL_ProcessPendingEvents.  There 
is no such facility on other platforms to do this: on other platforms must 
process the q explicitly.

Comment 1

[Chris Waterson]

I'm seeing this problem on Linux where almost *no* UI events are serviced while
the NSPR event queue is flooded; for example, I cannot scroll the document while
it's loading -- this works fine on Win32. Is this related?

Comment 2

[Stuart Parmenter]

correct.  the way PLEvents are handled is completly different on different
platforms.  On windows, (I think) events are sent to the win message pump when
they are posted.  On Linux, this isn't the case since there is no global message
queue.  If you want events to get processed, uh...  well...  hrmp.  nsAppShell
does this... (widget/public/nsAppShell.idl,
widget/src/{gtk,mac,windows}/nsAppShell*)... on linux (i'm not sure this will
work on mac (or windows...)), you could create an AppShell and call
ListenToEventQueue on it and then throw away the appshell...  this only works
probably because of an implimentation detail, and not because it was designed
exactly for this... you might want to ask danm what the correct way for someone
to listen to an event queue is... maybe he knows a good way.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Wasn't rationalizing this part of the idea behind the XP Event Loop stuff 
<http://www.mozilla.org/projects/xpcom/eventloop/> that Travis started but
didn't finish?  It would be a fine thing to make this code more XP.

Comment 4

[Doug Turner (:dougt)]

no.  that was for native event loops.  I don't think there was any effort to 
merge plevent handling into this.  Or at least there is no indication in the 
implementation.

Comment 5

[Warren Harris]

I think Travis' goal was to unify them so that you could wait on native and pl 
events at the same time (among other things).

Comment 6

[Doug Turner (:dougt)]

added mac native event notification.  I would like to check this in.  sfraser 
has review it.

Comment 7

[Stuart Parmenter]

is there a patch?

Comment 8

[Doug Turner (:dougt)]

Attachment: mac native notification.

Comment 9

[Doug Turner (:dougt)]

Attachment: modal dialog love

Comment 10

[Doug Turner (:dougt)]

bliz, did you have a patch for the gtk cleanup?

Comment 11

[Christopher Blizzard (:blizzard)]

I did a couple of hours trying to get it working and had some success.  I've got
a random hang that I was trying to work out.

Comment 12

[Doug Turner (:dougt)]

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

Comment 13

[Doug Turner (:dougt)]

can I get someone to review please?

Comment 14

[Doug Turner (:dougt)]

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

Comment 15

[Chris Waterson]

cc'ing sfraser, who may be able to comment on the first part of the change.

Comment 16

[Judson Valeski]

plus.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Bug 36849 is relevant.

You really want reflow PLEvents to be processed before paints. According to 
Waterson's comments, it sounds like PLEvents should NOT be processed after input 
events.

Comment 18

[Doug Turner (:dougt)]

Rob, that is exactly how the mac works now!  Need to file a bug on this... 46821


This bug is for the normalization of plevent handling, not specific event loop 
implementation.  I still believe that posting an notification is the right thing 
to do on all platforms.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

With this patch, according to beard's comments in n.p.m.xpcom, all PLEvents will 
get lower priority than repaints. That's going to be bad.

Comment 20

[Doug Turner (:dougt)]

indeed that is bad.  Could we change the event processing to poke for events?

Comment 21

[Doug Turner (:dougt)]

beard sfraser ccarlen,  could you review the patches that I have attached for
checkin?   This is a cleaner way of doing nspr events.  

Comment 22

[Conrad Carlen (not reading bugmail)]

I put it into by Mac build and it worked. The performance was equal to the 
current scheme. This was according to crude timing of how long it took to load 
lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp and to scroll from 
top to bottom. Need to add to my embedding sample to see if it simplifies life 
any in that case.

Comment 23

[Conrad Carlen (not reading bugmail)]

I added this to my embedding sample and it worked. The problem is that, in terms 
of code that I had to add to the app vs. code that I was able to remove, it was 
a loss. I still have to call Repeater::DoRepeaters() and Repeater::DoIdlers()in 
my message pump - otherwise scrolling doesn't work. Repeater hides PLEvent 
handling on the Mac. With this patch, it is no longer hidden yet I still have to 
call Repeater methods.

Comment 24

[Jim Roskind]

PDT would like to know the user consequences surrounding this item.  Is this 
strictly a developer issue?  

Comment 25

[vidur (gone)]

Response to jar - this is not a developer issue. It has strong ramifications on 
responsiveness of the browser.

Note to dougt - doing the opposite of what Robert O'Callahan suggested has 
proved to be a good thing for responsiveness while loading documents. jst and I 
made the following changes:
1) Windows - Reverted change that gave higher priority to PLEvents over UI 
events. With the change, we were starving out the UI during page loads.
2) Linux - Lowered the priority of events related to the PLEvent pipe on which 
glib was doing a select. Again, this allowed UI events to come through where 
otherwise they were being starved out.
3) Mac - No changes made here, but it's horrible that we're processing all 
PLEvents for every iteration of the event pump (note also that we're doing a 
NS_WITH_SERVICE for each iteration!). Simon's #ifdefed tweaking that does a 
single PLEvent per iteration might help responsiveness, but is not ideal either.  
Processing via a native event would be best.

Comment 26

[Peter Trudelle]

PDT will leave this to the developer and super-review to use appropriate 
caution in attempting to fix this.  This is really scary.  P2

Comment 27

[Doug Turner (:dougt)]

Attachment: current mac intergration diff from conrad

Comment 28

[Phil Peterson]

Not holding PR3 for this. Marking nsbeta3-. Please nominate for rtm if we need 
these changes for Seamonkey RTM

Comment 29

[Mike Pinkerton (not reading bugmail)]

dougt wants someone to investigate if this is a win or INVALID so i'll be the
guinea pig. I'll do some instrumentation before and after to see if we're really
losing a lot of time in ProcessPendingEvents() and to see if this makes us
better or worse (my guess is no change or slightly worse, but i'm ready to be
pleasantly surprised).

does anyone know if appleEvent handler is native in post 8.6, or are we going to
get a hit from switching to 68K?

Comment 30

[Conrad Carlen (not reading bugmail)]

Mike - I already did this a while back. I don't remember the exact numbers - but
it was on the order of "slightly worse." I think (I hope) I passed this on to
dougt. The downside to this is what I posted here 2000-08-29 07:16. An app needs
to add an AppleEvent handler for the PLEvent AND still needs to call nsRepeater
methods so it complicates things but without a real gain.

Comment 31

[Mike Pinkerton (not reading bugmail)]

so danm has a patch in another bug that coalesces multiple event notifications
into one when there are already events on the queue. Would the stuff that
sfraser was mucking around with (only handling one event at a time) break if we
cut down the number of native events we received?

sfraser: what were the measurable benefits of the things you did?

Comment 32

[Mike Pinkerton (not reading bugmail)]

after running a slew of tests on jrgm's "Page Loading Times Test" page with
dougt's changes (5 cycles of 40 pages each) , page loading time _improves_ by
about 10% (4.1 seconds average with vs. 4.5 seconds w/out the changes). This is
on 9.04, fwiw, on a fast, isolated connection. Haven't tested the impact on
modem users or slower, real-world connections.

The main visible affect of this patch is that pages are displayed in their
entirety, not just the html followed later by images popping in. Perhaps it's
slower, thus hitting fewer reflows as things come in, ending up with a net speed
increase. Hard to say for sure, but this is certainly promising enough that I
think we should investigate what it would take to make this good for embedding.

What say you folks? 

Comment 33

[Mike Pinkerton (not reading bugmail)]

danm has an additonal patch for plevent that makes things a little faster still
(down to 4.0 seconds average in the tests).

it seems to feel a lot snappier with these two patches applied, probably because
everything comes in at once.

Comment 34

[Conrad Carlen (not reading bugmail)]

Sounds great. Either some other things have changed since I tested this, or it's
because I am  on a slower connection. Where do I get jrgm's "Page Loading Times
Test" page? I'd be curious. If we do get a performance boost, I am in favor.

Comment 35

[Simon Fraser [no longer active]]

I never tested the effects of my handling 1 event at a time stuff. It was 
supposed to make chrome more responsive when loading large pages. If, after these 
recent changes, we don't lock up the chrome, then things should be OK.

Comment 36

[John Morrison]

The test is currently hosted at http://jrgm.mcom.com/page-loader/loader.pl, but 
will be moving to another server at some point soon. Contact me if you want
to run it and we'll work out a time. (I want to avoid having two people collide 
with multiple tests being run concurrently, which may skew the results. 
(It may have no effect, but I don't know for sure right now).) I'll also likely 
clean up the stuff so it can be installed on any old server.

Comment 37

[Mike Pinkerton (not reading bugmail)]

i noticed this morning that the patch totally hoses downloading files of any
kind (even SaveAs).

doug? any ideas?

Comment 38

[Peter Trudelle]

->moz0.9

Comment 39

[Mike Pinkerton (not reading bugmail)]

Hoo boy.

I instrumented the before and after code to see how this actually worked and 
found why we're faster on the pure layout tests. The gist is that posting all of 
these high level events starves our processing of update events, which is why we 
don't see the page draw until much later than normal. In fact, in many cases, we 
don't do a _single_ paint until _after_ we fire the onEndDocumentLoad event 
(where the elapsed time is returned to the server). Not doing any painting would 
certainly contribute to speeding up jrgm's tests. 

We are correctly invalidating window regions during the layout phase so we should 
be getting update events when we get back to the event loop. Furthermore, we are 
calling WaitNextEvent() during layout so, to address buster's comment, nisheeth's 
async code appears to be working.

We could play with events to pull out update events while we're processing high 
level events, but really, what's the point? Both simon and I think that we could 
do a better job (and not starve updates) by rewriting the event processing code 
to use globals that we just poll, like today.

adding helpwanted, futuring so we don't lose the work.

Comment 40

[Christopher Blizzard (:blizzard)]

Funny, what you are describing here is exactly what my recent event patch does.
 It mixes painting and processing high level events.  I think that it makes the
browser much more responsive on linux.

Comment 41

[gordon]

Is your patch xp, or just linux?

Comment 42

[Christopher Blizzard (:blizzard)]

Oh, it's just linux.  Linux is the only one that suffers from the unique problem
of starving file descriptors.

My kingdom for real message passing!

Comment 43

[Doug Turner (:dougt)]

why isn't think patch in the tree yet?  Are there problems with it?  

Comment 44

[Simon Fraser [no longer active]]

Read up, doug.

Comment 45

[Conrad Carlen (not reading bugmail)]

There was bug 76722 which was about layout yielding to events more often. I
wonder what effect that would have on this - specifically what pink pointed out
on 2001-02-22. 

Comment 46

[basic]

remove self

Comment 47

[Markus Hübner]

What's the current status of this one?

Comment 48

[Conrad Carlen (not reading bugmail)]

I've got a patch which uses a custom Carbon Event for this. Looking good, I was
able to completely remove the event queue processing repeater from the toolkit.
And, was able to remove the call to nsRepeater::DoRepeaters from PPEmbed. Needs
some perf testing. Patch if testing says it's good.

Comment 49

[Conrad Carlen (not reading bugmail)]

Attachment: uses custom Carbon Event handler

Comment 50

[timeless]

Comment on attachment 92928 [details] [diff] [review]
uses custom Carbon Event handler

>Index: xpcom/threads/plevent.c
>@@ -54,8 +54,9 @@
> 
> #if defined(XP_MAC)
> #include <AppleEvents.h>
> #include <Processes.h>
>+#include <CarbonEvents.h>
> #if !defined(XP_MACOSX)
> #include "pprthred.h"
> #endif
> #else
this doesn't look very !TARGET_CARBON safe

Comment 51

[Conrad Carlen (not reading bugmail)]

Plenty safe - the file is in Universal Interfaces and can still be included from
a Classic build. There are already too many #ifdefs in here to add one which
only saves inclusion of a file on a Classic build.

Comment 52

[Mike Pinkerton (not reading bugmail)]

cc'ing bryner so he'll stop asking me what bug# this is ;)

Comment 53

[Conrad Carlen (not reading bugmail)]

Made an opt build and ran the page loader test (5 cycles, cacheable, deleting
cache dir before each)

without patch: 1725
with patch: 1761

So, no gain, slight loss :-/

However, the reason I looked into this was so that embedding apps wouldn't have
to link with nsRepeater in widgetSupport. If I can tweak a little better perf
out of it so that it's even with current perf, I want to use this. Linking with
nsRepeater, which is needed only on Mac, isn't good.

Comment 54

[Conrad Carlen (not reading bugmail)]

Attachment: patch to xpcom

Comment 55

[Conrad Carlen (not reading bugmail)]

Attachment: patch to widget

Did some more perf testing on Mach-0 build. There was no penalty for this there
(1383 vs. 1384 on pageloader test). Looking for reviews and to get this in.

Comment 56

[Conrad Carlen (not reading bugmail)]

-> 1.3a

Comment 57

[Conrad Carlen (not reading bugmail)]

I need this for Mach-0 embedding work I'm doing. I don't want to drag nsRepeater
into that world and, with this, it's unescesary.

Comment 58

[Simon Fraser [no longer active]]

This code won't be built for Mach-O at all, since XP_MAC is not defined. It
would be interesting to see if it works for something like Chimera, tho.

Comment 59

[Conrad Carlen (not reading bugmail)]

Comment on attachment 102346 [details] [diff] [review]
patch to xpcom

It does get built for Mach-0 because at the top of plevent.c, there's a skanky
bit which #undefs XP_UNIX and #defines XP_MAC for XP_MACOSX. I'm going to clean
that up  and post a new patch.

Comment 60

[Conrad Carlen (not reading bugmail)]

Attachment: patch

All in one patch now. I removed the #ifdef fakery that was being done before in
plevent.c That, of course, required tons of #ifdef changes :-/

Comment 61

[Conrad Carlen (not reading bugmail)]

Attachment: patch v1.3

This patch clears events out of the native event queue which belong to a
PLEvent queue that is being destroyed. Previous patch would crash if a
PLEventQueue was destroyed and there were lingering events. Could happen on
closing a modal dialog.

Comment 62

[Conrad Carlen (not reading bugmail)]

Comment on attachment 109167 [details] [diff] [review]
patch

Clearing review requests for obsolete patch.

Comment 63

[Steve Dagley]

Comment on attachment 109560 [details] [diff] [review]
patch v1.3

Mongo like CarbonEvents

Comment 64

[Simon Fraser [no longer active]]

Comment on attachment 109560 [details] [diff] [review]
patch v1.3

Fix the spacing here:

+    OSErr err;
+    EventRef newEvent;
+    if (CreateEvent(NULL, kEventClassPL, kEventProcessPLEvents,
+		     0, kEventAttributeNone, &newEvent) != noErr)
+	 return PR_FAILURE;
+    err = SetEventParameter(newEvent, kEventParamPLEventQueue,
+			     typeUInt32, sizeof(PREventQueue*), &self);
+    if (err == noErr) {
+	 err = PostEventToQueue(GetMainEventQueue(), newEvent,
kEventPriorityLow);
+	 ReleaseEvent(newEvent);
+    }
+    if (err != noErr)
+	 return PR_FAILURE;

Comment 65

[Conrad Carlen (not reading bugmail)]

Checked in. BTW, on a final run of perf tests for this on my slow connection
(Mach-0 build), this patch gave a big boost on the jrgm pageloader test:
1612 vs. 1961 :-) Pages are drawing and flowing incrementally - it's not the
false speedup of the old patch that Pink explained in comment 39. That's just a
bonus - the point of this was not perf, but to avoid having apps poke the
PLEvent queue.

I would also like to get perf testing of PSM-related code.

Comment 66

[Markus Hübner]

Will this boost cross-platform pageload times?

Comment 67

[Conrad Carlen (not reading bugmail)]

No. Read comment 0. This provides that facility on Mac. Maybe this is still an
issue on Linux? If so, somebody can file a new bug.