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.

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).

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.

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.

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

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...

Sounds good. > I don't think anyone really cares about #4. My screenshot-based regression tester does.

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...

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 :-).

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

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>).

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?

Taking.

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 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...

> 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.

> 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.

(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! :-)

Checked in.