262760 - We should batch view updates around all reflows

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

FlushPendingNotifications sometimes does a Begin/EndUpdateViewBatch around
ProcessReflowCommands. It should always do it. Also, ReflowEvent::HandleEvent
dispatches asynchronous reflows and there should be a Begin/EndUpdateViewBatch
around that reflow processing, too. This should improve performance, and it
should be fairly safe since we are already doing the batching around all sorts
of reflows. It will also let me roll widget change caching into
Begin/EndUpdateViewBatch and have things just work.

Comment 1

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

Attachment: like so

Comment 2

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

Comment on attachment 160980 [details] [diff] [review]
like so 

seems to work

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 160980 [details] [diff] [review]
like so 

>Index: layout/html/base/src/nsPresShell.cpp
>+    // Style reresolves not in conjunction with reflows can't cause
>+    // painting or geometry changes

Is that true?  Note that a style reresolve can call into UpdateViewsForTree()
(in nsCSSFrameConstructor.cpp), which can end up calling UpdateView() (nosync,
though) and SyncFrameViewProperties().

At the moment we do a view update batch in ApplyRenderingChangeToTree() around
all this, but that means one batch per reresolve we process.  So if we know
we'll be processing multiple reresolves in a row (as here), it may make sense
to do an update batch around the whole thing.

Also, a reresolve can read to reframing, which means view (and widget,
possibly), creation.  I assume that we'd want to batch updates around that, no?

>@@ -6078,13 +6084,16 @@ struct ReflowEvent : public PLEvent {

>+        viewManager->BeginUpdateViewBatch();
>         ps->ProcessReflowCommands(PR_TRUE);
>+        viewManager->EndUpdateViewBatch(NS_VMREFRESH_IMMEDIATE);

Why is this immediate?	Shouldn't it be NOSYNC?

Comment 4

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

> Is that true?  Note that a style reresolve can call into UpdateViewsForTree()
> (in nsCSSFrameConstructor.cpp), which can end up calling UpdateView() (nosync,
> though) and SyncFrameViewProperties().

Nope. Forgot about that. Great, that will simplify the code :-). I'll revise the
patch.

> So if we know we'll be processing multiple reresolves in a row (as here), it
> may make sense to do an update batch around the whole thing.

Are there other entry points to the reresolver that we should bracket with
Begin/EndUpdateViewBatch?

> +        viewManager->EndUpdateViewBatch(NS_VMREFRESH_IMMEDIATE);

I thought I had a reason for this but I can't think of it now. I think you're
right, it should be NO_SYNC.

We should also add batching around other reflow points --- at least in
nsPresShell::ResizeReflow. Probably should do nsPresShell::InitialReflow too
because even though painting will be suppressed, widget geometry changes will
not be, and should be.

Hmm. Conceptually we should just be doing Begin/EndUpdateViewBatch around every
handling of an event. I wonder if we can just do that somehow.

Comment 5

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #4)
> Are there other entry points to the reresolver that we should bracket with
> Begin/EndUpdateViewBatch?

Hmm...  Not to the reresolver, no.  There are other entry points
ProcessRestyledFrames, though (which are doing restyles for reasons other than
hover/active changes and attribute changes).  See PresShell::RecreateFramesFor,
nsIPresShell::ReconstructStyleDataInternal, PresShell::Observe.  That seems to
be it.

> > +        viewManager->EndUpdateViewBatch(NS_VMREFRESH_IMMEDIATE);
> 
> I thought I had a reason for this but I can't think of it now.

Was it because you were going to reenable widget changes at EndUpdateViewBatch
and hence would want to paint immediately to prevent widget flicker?

> We should also add batching around other reflow points --- at least in
> nsPresShell::ResizeReflow. Probably should do nsPresShell::InitialReflow too
> because even though painting will be suppressed, widget geometry changes will
> not be, and should be.

Yeah...

> Hmm. Conceptually we should just be doing Begin/EndUpdateViewBatch around
> every handling of an event. I wonder if we can just do that somehow.

Hmm... One issue is that if we're inside a nested view batch and someone does
EndUpdateViewBatch(NS_VMREFRESH_IMMEDIATE) we won't refresh (since the outer
batch is not done yet).  So if we wrap the world in a batch, immediate refresh
is effectively turned off.  I guess we could change the handling of
NS_VMREFRESH_IMMEDIATE...

Comment 6

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

Yeah, we should record in the view manager whether anyone requested IMMEDIATE
painting, and if they did, then we paint immediately when we finally leave the
view batch.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 160980 [details] [diff] [review]
like so 

Marking r-, per comments.

Comment 8

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

I made the changes but adding batching to ResizeReflow makes resizing the
application window feel significantly slower. I'm going to have to do some
profiling to find out why.

Comment 9

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

Woo. I found something pretty interesting that I hadn't really realized before,
or maybe I did and forgot.

There are actually three different levels of immediacy in painting that we can have:
1) most immediate: we do some invalidation followed by a call to
nsViewManager::Composite, which synchronously updates the window(s) before we
return to the event loop.
2) intermediate: we do some invalidation but do not call Composite before
returning to the event loop. We will repaint when the toolkit issues an expose
event (which will happen *before* PLEvent processing, and GTK2 seems to ensure
exposes take priority over resize events, to ensure updates happen during resize)
3) least immediate: we suppress invalidation, storing dirty areas in views, and
post an Invalidate PLEvent. The Invalidate event gets processed after toolkit
events such as window resize events!

If you do nsIFrame::Invalidate or nsIViewManager::UpdateView, we will do 1) or
2) depending on whether you specify IMMEDIATE or NO_SYNC. But if you do
nsIViewManager::Begin/EndUpdateViewBatch, or
nsIViewManager::Disable/EnableRefresh, we will store everything up and then do
1) or 3) depending on whether you specify IMMEDIATE or NO_SYNC.

So normally when we're resizing, reflow is doing nsIFrame::Invalidates NO_SYNC,
which means the widget is invalidated immediately but no Composite is performed,
but GTK2 still arranges for exposes to be done in a timely manner. When I add
Begin/EndUpdateViewBatch NO_SYNC around the ResizeReflow, we're pushing
everything off into the Invalidate event which doesn't get handled until we stop
resizing. OTOH if I do Begin/EndUpdateViewBatch IMMEDIATE around the
ResizeReflow, we do a synchronous paint and then apparently GTK2 paints us again
for some reason (failing to coalesce our synchronous paint with its own resize
exposes?) and performance goes through the floor.

So I think what we need to do is to allow callers of EndUpdateViewBatch to
request any one of 1), 2) or 3). I think I'll do this by making IMMEDIATE
continue to mean 1), NO_SYNC means 2), and I'll introduce DEFERRED meaning 3),
only available for Begin/EndUpdateViewBatch and Disable/EnableRefresh.

Comment 10

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

Attachment: fix

Here's the final patch with all these changes. I changed all
EndUpdateViewBatch/EnableRefresh calls that took NO_SYNC to use DEFERRED, so
they should keep the same behaviour. (I actually doubt they all really *want*
DEFERRED, but that's an issue for another day.)

Comment 11

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

Attachment: fix

Here's the final patch with all these changes. I changed all
EndUpdateViewBatch/EnableRefresh calls that took NO_SYNC to use DEFERRED, so
they should keep the same behaviour. (I actually doubt they all really *want*
DEFERRED, but that's an issue for another day.)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 161345 [details] [diff] [review]
fix

>Index: view/src/nsViewManager.h
>+  PRInt32           mUpdateBatchFlags;

PRUint32 if it's being used as a bitfield, please.

I just realized I forgot an entry point to ProcessPendingRestyles.. see
http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSFrameConstru
ctor.h the HandleEvent() method of struct RestyleEvent.  It's probably
worthwhile to add view batching there too.

Please file a followup bug (or leave this one open?) for the DEFERRED issue?  I
do think most of those callers would prefer NO_SYNC -- they're just batching
because they expect a bunch of changes to happen all in a row, not because they
want to delay the paints.  If you don't have time to look into this, please let
me know and I will.

r+sr=bzbarsky with all that.

Comment 13

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

great, thanks.

Comment 14

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

Fix checked in.

Bug 263569 filed about the users of DEFERRED.