From the "3/24/09 1:22 AM" post by me in the "Re: Table internal frames and whitespace" thread in m.d.t.layout: On a related note, the eager nature of frame construction and the fact that some cases reconstruct the parent means that there are situations where removing a bunch of kids is O(N2) in the number of kids (each removal causes a reconstruct of all the kids). I wonder whether it's worth it to look into posting a restyle event to handle the parent reconstruct in MaybeRecreateContainerForFrameRemoval and just removing the frame normally. We can't do that sort of thing as easily on insertions, since we might not have a good place to put the new frame, but it'd work for removals... I've been thinking about it some more, and I think we do want to do this for insertions. The attached testcase shows why. Note that this is not a theoretical issue; since we now reconstruct the ancestor on various whitespace-related insertions inside tables (see bug 484448), the "notify separately on every node" behavior of setting innerHTML can easily lead to O(N^2) behavior when setting innerHTML in a table. For a good example, just try running our chrome mochitests and see how long it takes to load the front page; a profile shows that this is precisely the problem it's hitting. So what I think we want to do is the following: 1) On removal, if MaybeRecreateContainerForFrameRemoval returns true post a frame reconstruct via the normal async style change codepath for the relevant ancestor node. If the removal is part of a frame reconstruct, don't bother doing the ContentInserted (but see below; this might be unnecessary). 2) On insert/append if WipeContainingBlock returns true post a frame reconstruct and bail out. 3) On insert/append, if the parent (before resolving xbl stuff? probably good enough) already has a frame reconstruct posted against it, just bail out. This should be cheap enough, hopefully. It won't help with inserts further down in the subtree though, so it might not be worth it.

I didn't convert the mathml and fieldset parent reconstructs to async. Do we want this too? I don't know if we want to call MaybeRecreateContainerForFrameRemoval from RecreateFramesForContent because MaybeRecreateContainerForFrameRemoval might post a reconstruct, and RecreateFramesForContent is what handles posted reconstructs. And now some performance numbers. Using the testcase in this bug: without patch 2 19 4 84 8 201 16 616 32 2262 64 8623 with patch 2 5 4 5 8 10 16 19 32 34 64 75 128 132 256 247 512 484 1024 1090 Using the testcase from bug 501847 cut down to one tenth the size, clean build took 70 seconds to load, async containing block reconstruction but without the early bail in ContentInserted/Appended if parent has pending reconstruct took 59 seconds, and with the early bail took 42 seconds. Comparing the early bail vs not on the testcase in this bug showed about the same percentage difference.

I don't think you can just bail out from ContentRemoved after posting the async event. You want to synchronously remove the frame, while posting the async event to get the other frames in the tree "fixed" to deal with that removal...

I was just thinking some more, and would it make sense to just remove frames eagerly in the cases when we do the reconstruct now, but do the creation lazily? That is, add an arg to RecreateFramesForContent to say whether to do the insert sync. That would have the salutary benefit of not needing the extra early bail code, since we'd bail out in the normal way.

Used the idea from comment 4.

A few more thoughts: 1) The behavior of "wip" and "wip 2" differs for reconstructs coming from restyle event processing. "wip" made those async, which is wrong without other changes to restyle processing (and it should be possible to write a test that detects that, actually: change style in a way that would cause a reconstruct of a node which then would trigger reconstruct of its containing block, then read layout information that would depend on the new styles; with "wip" I would expect this to get wrong layout information). "wip 2" doesn't have this problem, at the cost of having the performance issue this bug is about when dealing with frame reconstructs coming from restyles. It might be better to make all the containing block reconstructs (including ones called from RecreateFramesForContent) async, and in restyle processing change the setup to keep going until there are no pending restyles. 2) The second argument to PostRestyleEvent should be nsReStyleHint(0), since we don't actually want to force style data recomputation.

Not that item 2 matters much, given that we're doing a frame reconstruct anyway.... But in case we ever change those to not automatically recompute data...

Process any pending restyles generated by processing restyles in ProcessPendingRestyles. Changed the ReStyleHint argument to PostRestyleEvent.

The point was that once you do the restyle processing, we can make the all the functions involved other than RecreateFramesForContent always async.

Whoops. All the other functions were actually async because they were only called with async=true, but I did a good job of hiding that by leaving the extra arguments on those functions.

Comment on attachment 391696 [details] [diff] [review] wip 4 >+ // loop so that we process any restyle events generating by processing generated Why use a default for the parameter? It seems like you're adding PR_TRUE to over half the callers, and new callers seem at least as likely to want async as sync. Shouldn't all the callers have to choose explicitly?

(In reply to comment #11) > (From update of attachment 391696 [details] [diff] [review]) > >+ // loop so that we process any restyle events generating by processing > > generated Ok. > Why use a default for the parameter? It seems like you're adding PR_TRUE to > over half the callers, and new callers seem at least as likely to want async as > sync. Shouldn't all the callers have to choose explicitly? Can change that. No real reason.

Could you post a new patch with that fixed? I'd sort of like to see all the callers as I review.

Addressed the two issues in comment 11.

Comment on attachment 391945 [details] [diff] [review] patch r=dbaron Any reason you didn't want to do the frameset, MathML, and legend cases async too? This seems to get us a good bit of the way to bug 502937. (Switching to PR_TRUE for the ProcessRestyledFrames / RestyleElement cases would be part of that.) I haven't managed to think of anything that would go wrong with the undisplayed map with this. The one thing I'm worried about a bit is interleaving with content sink notifications in a bad way. Are there cases where additional content could be in the tree by the time we process the restyle that hasn't been notified on yet?

Comment on attachment 391945 [details] [diff] [review] patch bz thinks this wouldn't introduce any new content sink flushing issues that we didn't already have, and I'm pretty sure he's right.

(In reply to comment #15) > Any reason you didn't want to do the frameset, MathML, and legend cases async > too? There should only a few frames involved with framesets, so no performance gain to be had there. Same with legend. I don't know enough about MathML, so I didn't want to change it.

http://hg.mozilla.org/mozilla-central/rev/736ada2d64bf Thanks!!!

Mass change: adding fixed1.9.2 keyword (This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)