Closed Bug 262760 Opened 15 years ago Closed 15 years ago

We should batch view updates around all reflows

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

References

Details

(Keywords: perf)

Attachments

(2 files, 1 obsolete file)

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 on attachment 160980 [details] [diff] [review]
like so 

seems to work
Attachment #160980 - Flags: superreview?(bzbarsky)
Attachment #160980 - Flags: review?(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?
> 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.
Blocks: 238493
(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...

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 on attachment 160980 [details] [diff] [review]
like so 

Marking r-, per comments.
Attachment #160980 - Flags: superreview?(bzbarsky)
Attachment #160980 - Flags: superreview-
Attachment #160980 - Flags: review?(bzbarsky)
Attachment #160980 - Flags: review-
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.
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.
Attached patch fix (obsolete) — Splinter Review
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.)
Attached patch fixSplinter Review
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.)
Attachment #161345 - Flags: superreview?(bzbarsky)
Attachment #161345 - Flags: review?(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.
Attachment #161345 - Flags: superreview?(bzbarsky)
Attachment #161345 - Flags: superreview+
Attachment #161345 - Flags: review?(bzbarsky)
Attachment #161345 - Flags: review+
Fix checked in.

Bug 263569 filed about the users of DEFERRED.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Keywords: perf
You need to log in before you can comment on or make changes to this bug.