144072 - [FIX]Reduce cost of FlushPendingNotifications

Status: RESOLVED FIXED

(SeaMonkey :: General, defect, P1)

Description

[John Keiser (jkeiser)]

When you want to access content lists, there is a high cost because you must
flush the content sink.  When it does this, it flushes into the
nsCSSFrameConstructor.  You should be able to access purely content things
without triggering the creation of frames and subsequent reflow.  This is bad
because the more often you do it the more unnecessary reflow you will end up doing.

Notifications need to be rethought anyway.  Currently the way we flush, we
perform a ContentAppended every time a container closes, if that container had
any children.  This is a rather arbitrary way of handling the flushing.  I
believe the content sink should flush everything immediately and let each
individual observer sort out what 

This bug will track changes and ideas as I or others come up with them.  Here is
my current thinking on how to deal with this:

1. Add children into their containers' children list immediately upon creating
the children.

2. Send every single append, insert and removal immediately from the sink to the
document.  Let the document handle it its own way.  (This could be done in its
own step by making the document take over the burden of flushing.)

3. Make the document observer send the notifications on immediately and let each
observer handle the notifications in their own way.
   Content lists can be fixed to handle appends either immediately or lazily,
both of which would make it so they can receive notifications immediately and
therefore anyone who is looking for a content list no longer has to flush.
   nsCSSFrameConstructor, which really should batch to be effective, should
queue up the notifications and flush on its own time (either when someone
flushes frames specifically--if they need the frame for some reason--or every
quarter of a second, or something).  This, along with sending notifications
immeidately from the sink and document, would get rid of bug 75001 completely by
pushing it to a real timer.

Comment 1

[John Keiser (jkeiser)]

This bug will eventually fix bug 75001 and will fix 138892 beautifully (we can
even remove lines from that function when this happens).  I have a suspicion it
would allow the Link Toolbar performance not to such (I heard it was pulled out
of 1.0 because it added 3-4% on pageload due to document listener foo).

Comment 2

[John Keiser (jkeiser)]

Attachment: Patch: Set child lists immediately

This patch is incomplete for two reasons:
- I have not removed the places that call Flush just so that child lists could
be updated.
- there is a FlushText missing from places that do AddContainerToParent.

That said, this patch does not increase pageload time on my computer, which is
encouraging.

Comment 3

[John Keiser (jkeiser)]

Making a note here that we fixed bug 138892 to work around this, and we are
going to work around bug 166752 as well.  Let us back those guys out if and when
we fix this.

Comment 4

[John Keiser (jkeiser)]

It is very sad, but this will have to wait.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I think jkeiser had the right idea here.  At the moment,
FlushPendingNotifications flushes the following:

1)  The sink's content model
2)  The frame model
3)  All pending reflows (including ones generated in #2)
4)  All pending viewmanager invalidates (note that this does not do a sync
    paint, at least on Linux, since the widget code isn't told to do that; the
    invalidates are just posted to gtk off a flush).

I'm proposing to add a step in between 1 and 2 (or maybe 2 and 3) which will be
style reresolves (bug 230170).

A lot of DOM consumers only care about #1 on this list; a lot of other DOM
consumers only care about #1 and #2.  Some places actually want #3.  I don't
think anyone really cares about #4.

Minimally intrusive would be changing FlushPendingNotifications() to take a
bitfield that would only cause flushes of the relevant things...  we can have
convenience values (like "layout" for (content | frames | reflow)) or something.

Alternately, we could take jkeiser's approach and make some major changes to the
HTML sink; we'd still need the bitfield, though.

Thoughts?  I've seen a bunch of DHTML profiles where FlushPendingNotifications()
is really hurting performance, so in the process of doing this it would be worth
evaluating where we really need to flush too...

Comment 6

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

Sounds good.

> I don't think anyone really cares about #4.

My screenshot-based regression tester does.

Comment 7

[Boris Zbarsky [:bzbarsky]]

roc, if it cares about those, how does it work?  Note, again, that #4 doesn't
actually paint; it just flushes invalidates out to the widget layer...

Also, the thoughts I'm looking for are on which of the two approaches to take
wrt the HTML sink...

Comment 8

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

http://lxr.mozilla.org/mozilla/source/content/base/src/nsDocumentViewer.cpp#941
and thereabouts calls nsPresShell::FlushPendingNotifications(PR_TRUE) which
calls nsViewManager::EndUpdateViewBatch(VM_REFRESH_IMMEDIATE) which calls
nsViewManager::Refresh(VM_REFRESH_IMMEDIATE) which calls Composite() which
actually paints.

I know very little about the content side, and I plan to keep it that way, sorry
:-).

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ah, I had missed the Composite() call....

Comment 10

[Boris Zbarsky [:bzbarsky]]

OK, I did a tad of investigating.  It looks like the HTML sink already supports
flushing out tags to make the content model up to date without notifying the
document observers (though that codepath isn't actually called by anything at
the moment... but it does look correct).

So splitting up flushes as described shouldn't be _that_ bad.  One thing we
should decide first, though, is which flushes imply which other ones.  At the
moment we have a setup where flushing reflows will first flush out content model
and frames.  Do we want to enforce that?  Or do we want to make it possible to
flush out pending reflows without flushing the other stuff?

Also, I looked over jkeiser's patch in this bug and I'm not sure I like it.  It
would insert nodes into the tree immediately (before their kids have been
parsed), which would break things that depend on not being inserted till after
that has happened (scripts) and make some things slower for nodes that handle
child insertion differently depending on whether they are in the document (eg
<style>).

Comment 11

[Boris Zbarsky [:bzbarsky]]

OK, so my proposal is as follows:

1)  Add a mozNotificationType enum, in its own header (a la nsChangeHint).
2)  Include this header in nsIContentSink, nsIDocument, nsIPresShell.
3)  Change the FlushPendingNotifications methods on those interfaces to take a
    mozNotificationType.
4)  Fix the impls and callers accordingly.

One issue here is exactly what nsIPresShell::FlushPendingNotifications should
do... at the moment, it just flushes reflows for that presshell, without
flushing reflows for the parent (needed for frames!) and uses a hack to flush
content.  In my opinion, there should be a bit in mozNotificationType that tells
the presshell "called from document" -- if this is not set, just call back to
the document, otherwise flush reflows, etc.  This seems simpler than trying to
legislate that no one but documents call nsIPresShell::FlushPendingNotifications.

Seem reasonable?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Taking.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Just change FlushPendingNotifications

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 149094 [details] [diff] [review]
Just change FlushPendingNotifications

Some notes:

1)  The restyle stuff is not used yet, but will be, so I figured I'd put it in
now.

2)  I left some code calling the presshell if it's sure it just wants to flush
reflows and doesn't want to flush for the parent documents.

3)  Updating content from inside frames is generally bad (since it may destroy
the frame), so I just have frames flushing pure reflows if they think they need
an up-to-date presentation.

Brief summary of patch:

nsIPresShell and nsIDocument still both have the flush method, but the
nsIDocument one is preferred in almost all cases (as the nsIPresShell comments
say).  I'll probably add some docs to the nsIDocument method too (if nothing
else, mention that if flushes ancestor docs...).

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 149094 [details] [diff] [review]
Just change FlushPendingNotifications

+enum mozFlushType {
+  Flush_Content	   = 0x1,   /* flush the content model construction */
+  Flush_SinkNotifications = 0x2,   /* flush the frame model construction */
+  Flush_StyleReresolves   = 0x4,   /* flush style reresolution */
+  Flush_OnlyReflow	   = 0x8,   /* flush reflows */
+  Flush_OnlyPaint	   = 0x10,  /* flush painting */
+  Flush_Frames 	   = (Flush_Content | Flush_SinkNotifications),
+  Flush_ContentAndNotify  = (Flush_Content | Flush_SinkNotifications),
+  Flush_Style		   = (Flush_Content | Flush_SinkNotifications |
+			      Flush_StyleReresolves),
+  Flush_Layout 	   = (Flush_Content | Flush_SinkNotifications |
+			      Flush_StyleReresolves | Flush_OnlyReflow),
+  Flush_Display	   = (Flush_Content | Flush_SinkNotifications |
+			      Flush_StyleReresolves | Flush_OnlyReflow |
+			      Flush_OnlyPaint)
+};

Couldn't this live in nsIDocument in stead of in its own file (not that I
really care)?

And Flush_Only*, why the "Only" in the name? Seems inconsistent with the
others... And Flush_Frames and FlushContentAndNotify are the same, can one be
removed?

- In nsHTMLDocument::FlushPendingNotifications():

+      // XXX Ack! Parser doesn't addref sink before passing it back
+      sink = mParser->GetContentSink();

Ah, it's already deCOMtaminated! :-) Wanna just remove that comment, and make
sink a raw pointer (or should we do the right thing here and hold a strong
reference to the sink while flushing it)?

+      if (sink) {
+	 PRBool notify = ((aType & Flush_SinkNotifications) != 0);
+	 nsresult rv = sink->FlushContent(notify);
+	 if (NS_FAILED(rv))
+	   return;
+      }

Is it really worth checking rv here, really? Maybe
nsIContentSink::FlushContent() should simply be a void function?

- In nsDocument::FlushPendingNotifications():

+  if (aType == (aType & (Flush_Content | Flush_SinkNotifications)) ||

if (!(~aType & (Flush_Content | Flush_SinkNotifications)) ||

Not sure what's easier to read :-)

- In DocumentViewerImpl::LoadComplete():

   if (forcePaint) {
-    if (mPresShell) {
-      mPresShell->FlushPendingNotifications(PR_TRUE);
-    }
+    mDocument->FlushPendingNotifications(Flush_Display);

This should only force painting of the current document, not necessarily its
parent, no? So wouldn't the right thing to do be to flush the presshell here,
and not the document?

- In nsHTMLBodyElement::GetBgColor():

     // XXX This should just use nsGenericHTMLElement::GetPrimaryFrame()
     if (mDocument) {
       // Make sure the presentation is up-to-date
-      mDocument->FlushPendingNotifications();
+      mDocument->FlushPendingNotifications(Flush_Style);
     }

     nsCOMPtr<nsIPresContext> context;
     GetPresContext(this, getter_AddRefs(context));

     if (context) {
       nsIFrame* frame;
       rv = context->PresShell()->GetPrimaryFrameFor(this, &frame);

As the comment says, shouldn't this just use
nsGenericHTMLElement::GetPrimaryFrameFor()?

- In nsHTMLExternalObjSH::GetPluginInstance():

+  // Have to flush layout, since plugin instance instantiation
+  // currently happens in reflow!  That's just kinda uncool.

Remove "kinda" from the last sentence :-)

- In GlobalWindowImpl::GetInnerWidth():

-  FlushPendingNotifications(PR_TRUE);
+  // If we're a subframe, make sure our size is up to date.  It's OK that this
+  // crosses the content/chrome boundary, since chrome can have pending
reflows
+  // too.
+  GlobalWindowImpl* parent = NS_STATIC_CAST(GlobalWindowImpl*,
+					     GetPrivateParent());
+  if (parent) {
+    parent->FlushPendingNotifications(Flush_Layout);
+  }

What if we're a parentless window (i.e. a modal dialog), we'd still want to
flush to get our size right if we're a size-to-content dialog, no? What's wrong
with flushing on the document? Maybe the document needs to just get its parent,
no matter what type its parent is? Or would that be way too expensive?

Oh, and while you're at it, wanna make GetPrivateParent() *not* return NS_OK?
:-)

- In GlobalWindowImpl::GetOuterWidth():

+  // XXXbz does this actually do anything?
+  GlobalWindowImpl* rootWindow = NS_STATIC_CAST(GlobalWindowImpl*,
+						 GetPrivateRoot());
+  if (rootWindow) {
+    rootWindow->FlushPendingNotifications(Flush_Layout);
+  }

What part are you wondering about, GetPrivateRoot(), or flushing it? I guess
the only case flushing would do anything is with size-to-content windows...

r+sr=jst with that thought about...

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Couldn't this live in nsIDocument in stead of in its own file

I'd need to change a bunch of forward-declarations of nsIDocument (eg in
nsIPresShell) into actual includes.  And nsIDocument pulls in all sorts of
stuff, so I'd need to change REQUIRES lines...

I could do it, if you want.

> And Flush_Only*, why the "Only" in the name?

To clearly differentiate them from Flush_Layout and Flush_Display.  If you have
better ideas on names for the "combined" vs "individual" flush types, I'd love
to hear them!

> Wanna just remove that comment, and make sink a raw pointer

We should probably hold a ref to it while flushing it.

> Maybe nsIContentSink::FlushContent() should simply be a void function?

Good point.  Will do so.

> if (!(~aType & (Flush_Content | Flush_SinkNotifications)) ||

Maybe I should add a "FLUSH_TYPE_INCLUDES()" macro?

> - In DocumentViewerImpl::LoadComplete():

We want to flush content, layout, paint for the current document.  We don't want
to flush parents much at all, here....

Perhaps flush content only on the document, then flush display/reflow on the
presshell?

> As the comment says, shouldn't this just use
> nsGenericHTMLElement::GetPrimaryFrameFor()?

Oh, heh.  Yeah, will fix.

> we'd still want to flush to get our size right if we're a size-to-content
> dialog, no? What's wrong with flushing on the document? 

Consider a DHTML thing that positions stuff based on the window size, getting
the window size for every positioning operation (fairly common).  With this
code, we would flush reflows all over the place, leading to terrible
performance.  Since the window size is never affected by the contents of the
window except in the size-to-content case, I'd rather push the flushing out to
size-to-content.  See bug 244235.

This change is one of the major reasons I wanted to fix this mess, so we should
figure out this issue....

> Oh, and while you're at it, wanna make GetPrivateParent() *not* return NS_OK?

Sure.

> What part are you wondering about, GetPrivateRoot(), or flushing it?

Flushing.  You're right that for size-to-content windows it matters, so I'll
remove that comment.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> And Flush_Frames and FlushContentAndNotify are the same, can one be removed?

Forgot to address this.  They are the same in implementation, but different in
concept.  If we ever end up with async frame construction (not as directly tied
to content notifications), then they will in fact mean different things.  I used
them at callsites based on what the caller actually cares about -- notifications
or frames.

Comment 18

[Johnny Stenback (:jst)]

(In reply to comment #16)
> > Couldn't this live in nsIDocument in stead of in its own file
> 
> I'd need to change a bunch of forward-declarations of nsIDocument (eg in
> nsIPresShell) into actual includes.  And nsIDocument pulls in all sorts of
> stuff, so I'd need to change REQUIRES lines...
> 
> I could do it, if you want.

Nah, new file is fine.

> > And Flush_Only*, why the "Only" in the name?
> 
> To clearly differentiate them from Flush_Layout and Flush_Display.  If you have
> better ideas on names for the "combined" vs "individual" flush types, I'd love
> to hear them!

Hmm, nothing comes to mind, other than separating out the combinations from the
enum into macros whose names are all upper case, but that's not all that pretty
either...

> > Wanna just remove that comment, and make sink a raw pointer
> 
> We should probably hold a ref to it while flushing it.

Fair enough, but remove the comment.

> > if (!(~aType & (Flush_Content | Flush_SinkNotifications)) ||
> 
> Maybe I should add a "FLUSH_TYPE_INCLUDES()" macro?

If we see code like this spread out all over in the future, a macro might be a
good idea, but for now, I wouldn't worry...

> > we'd still want to flush to get our size right if we're a size-to-content
> > dialog, no? What's wrong with flushing on the document? 
> 
> Consider a DHTML thing that positions stuff based on the window size, getting
> the window size for every positioning operation (fairly common).  With this
> code, we would flush reflows all over the place, leading to terrible
> performance.  Since the window size is never affected by the contents of the
> window except in the size-to-content case, I'd rather push the flushing out to
> size-to-content.  See bug 244235.
> 
> This change is one of the major reasons I wanted to fix this mess, so we should
> figure out this issue....

Yeah, makes sense now. Fine with me.

Ship it! :-)

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 20

[Boris Zbarsky [:bzbarsky]]

Checked in.