240874 - Is it possible to speed up PostReflowEvent?

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Boris Zbarsky [:bzbarsky]]

PostReflowEvent repeatedly shows up in profiles of DOM manipulations.  The basic
structure of the function is:

nsCOMPtr<nsIEventQueue> eventQueue;
mEventQueueService->
   GetSpecialEventQueue(nsIEventQueueService::UI_THREAD_EVENT_QUEUE,
                        getter_AddRefs(eventQueue));
if (eventQueue != mReflowEventQueue &&
    !mIsReflowing && mReflowCommands.Count() > 0) {
  // Post event
  mReflowEventQueue = eventQueue;
}

I assume the idea here is that if someone pushes a new event queue and then we
get a reflow request we need to post a new event, since the old one may not get
processed for a while?  It seems to me that this is still broken in the case
when an event queue is pushed and then we get no reflow requests; we'll not get
to that reflow till the new queue is popped, will we?

Anyway, the point is that getting the thread event queue is expensive (monitors,
etc, etc).  So I was wondering what, if anything, we can do to speed up this
code....  In typical dom testcases, 99% of the time we get into this method we
already have an event posted and don't have to do anything; it's just that
figuring that out is just as expensive as doing something.

Comment 1

[Nicholas Allen]

Why can't the test for whether a reflow is needed just be moved before the test
on the event queue?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Because that test tests true almost every single time we end up in this code, so
it doesn't help much.

Comment 3

[Boris Zbarsky [:bzbarsky]]

This method is about 3% on some DHTML testcases I was looking at; if I fix bug
230170 the event queue stuff will probably go up to 5% or more (since we'll need
to post reresolve events too, and those will have the same issues).

One suggestion that was made on #mozilla was to have two event queues per thread
-- one modal, one modeless.  When a nested queue is pushed, it blocks events in
the modal queue, but NOT the modeless one.  Things that should not be affected
by nested queues (necko events?  reflow events?) go in the modeless queue. 
Things that should be blocked by nested queues (timer events? DOM events of
various sorts?) go in the modal queue.  When processing events we have to
load-balance the queues or something, but that may be worth it....

Comment 4

[Dan M]

Another possibility that I haven't thought all the way through:

It's almost never the right thing to do, cacheing an event queue because, as
you've noted, a new one could be pushed on top of your cached one at any time.
This code does the right thing, fetching it anew each time. If that's a speed
concern however it seems feasible for the EventQueue chain to send
nsIObserverService notifications when a queue is pushed or popped. Code like
this could then generally cache the queue, going through the EventQueueService
only when necessary. That might be faster. Queues are normally pushed or popped
at a low frequency.

I do have lifetime concerns. Queue cacheing could cause a dead queue to stall on
the chain. That shouldn't be a problem if every listener gets a crack at
refreshing its cached copy relatively quickly. Maybe the right thing to do upon
receiving the notification is to immediately let go of the cached queue, and
re-cache when next you need to access the current queue.

Dual event queues frighten me. You'd have to be very careful in your choice of
queue types for events, and test the living snot out of it. Reflow events would
seem to benefit from a modeless queue. Necko events are explicitly not good
candidates. It was for them that queue modality was invented.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> it seems feasible for the EventQueue chain to send nsIObserverService
> notifications when a queue is pushed or popped

If this is low-frequency (and I think it should be, as you say), then this would
solve the problem perfectly for reflow events.

> Code like this could then generally cache the queue

No need, actually.  We're only caching it because we use that to check whether a
new queue got pushed (by comparing the cached one to the current one).  If we
get notified when a queue is pushed, we have absolutely no need to cache a queue
in this code.

> Necko events are explicitly not good candidates. It was for them that queue
> modality was invented.

Really?  But people keep complaining that downloads in one window stall when an
alert comes up in another window.  And darin keeps complaining that popping a
queue processes events in the parent, thus introducing reentrancy that keeps
nested event queues from being a real event modality solution...

That discussion should probably go in a separate bug, though.  I agree that the
two-queue approach would be much more involved than just notifying the observer
service.

Comment 6

[Dan M]

Ah. Looks like this code delays the reflow if it sees another queue has pushed
its way in. Then push notifications would seem to help, and could potentially be
introduced into cached situations elsewhere in a leisurely way, after some
profiling.

>But people keep complaining
Yeah, it's not perfect. Thing is, you want to be able to stall I/O in a window
with a prompt, yet load the prompt. That's where the whole mess came from.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Thing is, you want to be able to stall I/O in a window with a prompt, yet load
> the prompt.

OK, fair enough.

> Then push notifications would seem to help, and could potentially be introduced
> into cached situations elsewhere in a leisurely way, after some profiling.

Exactly.  So far, I'd just need the push notifications here and in bug 230170
(will probably use the same observer for both, in fact).

Comment 8

[Brendan Eich [:brendan]]

Dogpiling on the architecture Q&A: why do we expose event queues, making for
stuck queues due to leaks and other hazards?  Why not abstract away the stack
element and make people hold the queue-stack, so it can route calls to the top
(or bottom) queue as it sees fit?

/be

Comment 9

[Dan M]

Sometimes that is what you want. There are places in the code that fetch the
active event queue each time they want to post an event. But sometimes you want
to be able to post to the precise event queue that represents your processing
sink, so that whatever you're trying to do can be stalled if necessary. I'm
pretty sure nsHttpChannel is an example.

Comment 10

[Boris Zbarsky [:bzbarsky]]

So.. this business of getting the current UI queue over and over means that
GetSpecialEventQueue is about 2-3% of time on a typical "dhtml" testcase.  For
comparison, all of reflow is 8-9%.

So if we can make some progress here, that would be nice...

Comment 11

[Markus Hübner]

Do we need a testcase here for further analysis?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Not particularly.  There are plenty of testcases in the bugs this blocks.

Comment 13

[Boris Zbarsky [:bzbarsky]]

This is fixed by the patch for bug 326273.  With that patch, PresShell::PostReflowEvent() is less that 0.1% of total time.