390425 - [FIXr]Simplify {ib} handling a bit

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

I was looking into bug 387227, and decided that the NeedSpecialFrameReframe mess was just too confusing... and not all that necessary.  The attached patch removes it.

More precisely, this patch just makes us go ahead and construct the kids... and then if they don't work out we handle that via WipeContainingBlock (which we were doing anyway).  Substantive changes:

1)  Minor tweak to the ContentAppended optimization to actually check the right
    thing about the child display.
2)  Removal of NeedSpecialFrameReframe.  This has an effect only in cases when
    we avoided a reframe by adjusting the parent.  In other words, only when
    we were inserting at the boundaries between the parts of an {ib} split in
    a particular way.  I don't think that optimizing that case merits the
    complexity we devoted to it.

The rest is cleanup that just happened as I was working on this.

This passes all the tests in the diff (which trunk does not, by the way), and does not regress bug 22413, bug 27211, bug 39211, bug 56894, bug 113235, bug 286491.

Note that we _might_ be able to keep the aInReinsertContent flag and skip the WipeContainingBlock call if it's true... but I'm not sure, and this is safer.  Now that we walk up to a block that's safe to wipe (we've done that for a while), we won't run into issues with recursion there in any case.

roc, I think you can r+sr this, but maybe it would be good to get Mats or dbaron to look as well.  Your call depending on how happy you are, ok?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to tip

Comment 2

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

+IsInlineFrame(const nsIFrame* aFrame)

Would it make more sense to just test IsFrameOfType(eLineParticipant) here? That seems more future-proof than testing GetType().

+      // XXXbz is this a useful optimization?  And is it really worth the
+      // complexity?

I think not.

+  // If aFrame is an inline, then it cannot possibly have caused the splitting.
+  // If the frame is being reconstructed and being changed to a block, the
+  // ContentInserted call will handle the containing block reframe.  So in this
+  // case, try to be conservative about whether we need to reframe.  The only
+  // case when it's needed is if the inline is the only child of the tail end
+  // of an {ib} split, because the splitting code doesn't produce this tail end
+  // if it would have no kids.  If that ever changes, this code should change.

Is this optimization worth it? Why not just reframe whenever the parent is special?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Would it make more sense to just test IsFrameOfType(eLineParticipant) here?

It's not quite equivalent.  In particular, your test is true for first-letter frames, but mine isn't.  That might be ok, but it'll need some code-tracing to make sure...  I can try to do that if you want.

> I think not.

Want me to just remove it then?

> Is this optimization worth it?

Good question.  It might not be.  We can take it out to simplify things, then if it ever comes up we can think about reinstating it...

Comment 4

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

> In particular, your test is true for first-letter
> frames, but mine isn't.

OK but you want to return true for first-letter, right? E.g. inserting a block inside a first-letter should split.

> Want me to just remove it then?

Yes.

> We can take it out to simplify things, then if it ever comes up we can think
> about reinstating it...

Let's do that. My intuition is that reframes aren't performance-relevant, except possibly reframes due to IB splits ... maybe there could be trouble with a series of appends to an inline that was turned into a block?

Comment 5

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

BTW does your patch fix bug 391628 by any chance?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> BTW does your patch fix bug 391628 by any chance?

Yes.  The remove-from-split-inline-2.html test in the patch is basically identical to the testcase in bug 391628.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> OK but you want to return true for first-letter, right?

Never comes up, since we remove letter frames before modifying the DOM.  In any case, I just checked all three callers of IsInlineFrame(), and all happen either when there are no letter frames around or after an explicit check for a letter frame.  So I'll make that change.

> maybe there could be trouble with
> a series of appends to an inline that was turned into a block?

That wouldn't hit this code anyway.  MaybeRecreateContainerForIBSplitterFrame is only called from ContentRemoved and RecreateFramesForContent.  So there could be a performance issue if a bunch of inline kids of the inline part of an inline split are removed one at a time or all restyled with frame reconstruct hints or something, with the style reresolves targeted at them individually, and not at any ancestor...  You're probably right that it's not really worth optimizing for.  I'll take it out.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 9

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

Comment on attachment 276085 [details] [diff] [review]
Updated to comments

+  NS_PRECONDITION(aFrame == aFrame->GetFirstContinuation(),

!aFrame->GetPrevContinuation() would be simpler/faster?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 276085 [details] [diff] [review]
Updated to comments

This patch makes the {ib} code more straightforward and easier to reason about.  It fixes a few correctness bugs.

Risk is medium.  The biggest risk is that we're removing a few optimizations, but we think they're fairly corner cases that won't affect things much in practice except in very contrived situations.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 276085 [details] [diff] [review]
Updated to comments

A bit scary, but a1.9=dbaron.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 13

[Boris Zbarsky [:bzbarsky]]

So this regressed Tp.  Specifically, it about doubled the time needed by the voodooextreme Tp test page.

I'm working on getting this built on my slow machine so I can do perf testing, but just breakpointing on the fast machine indicates two main callers to WipeContainingBlock:

1)  Appending to the special block for the outermost font, with an inline at the very end (which should therefore go into the non-existent ending inline, requiring a reframe).  This was a correctness fix that I made in this patch, but in this case it means doing a reframe on the body a few times during pageload.

2)  Appending a list including a block to a non-special inline.  Generally the first frame being appended is inline, so I doubt this affected things.  I'll test to make sure.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to put this back in.

I backed this out to fix Tp

Comment 15

[Boris Zbarsky [:bzbarsky]]

OK, so the issue is indeed all #1 above.  Basically, all the tables are inside a <font> and there's a textframe after the last table, so towards the end of the pageload we have to reconstruct it all.

So we either need to get rid of that minor correctness fix or try to do it without reconstructing.  I'll try for the latter in the append case, and leave the reconstruct in the insert case, I think.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to fix performance regression

Implement comment 15.

This fixes the performance issue locally for me, as far as I can tell.  It's a little complicated, unfortunately, though I think it's still more straightforward than what we used to have in NeedSpecialFrameReframe and the like.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Note: the diff against the "patch to put this back in" indicates which parts really need review...

Comment 18

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

Comment on attachment 276399 [details] [diff] [review]
Updated to fix performance regression

r+sr+a=roc assuming you fix the bug here we talked about:
+             (aNewParent->GetStateBits() & NS_FRAME_HAS_CHILD_WITH_VIEW));

Comment 19

[Boris Zbarsky [:bzbarsky]]

I checked that in, and on tinderbox it's showing a 200ms regression on voodooextreme (which basically accounts for the ~6ms Tp regression it's showing in general).  Unlike the first patch, Windows is not showing a regression.

Comment 20

[Boris Zbarsky [:bzbarsky]]

To be more clear, the tinderbox regression is Linux-only.

Comment 21

[Boris Zbarsky [:bzbarsky]]

OK.  So locally I always see WipeContainingBlock returning false on the voodooextreme test page.  Time spent under both WipeContainingBlock and AppendFrames is very small according to jprof.

However, the amount of time spent under nsCSSFrameConstructor::ReframeContainingBlock is about 10% of pageload locally (wouldn't necessarily account for the 25% I see on tinderbox, but given the difference in other performance characteristics between here and there, maybe it might).  These calls are coming from nsCSSFrameConstructor::MaybeRecreateContainerForIBSplitterFrame when we reframe an image... and the reason we reframe the images is that their state changes from loading to not loading.  Now this particular page happens to be in quirks mode, so we might be able to avoid the reframe, since we have an image frame already.  But if it were standards mode we couldn't get away with that.

In any case, the issue is that we're reframing an inline image that is the child of a block part of an {ib} split.  Oh, and the containing block of that block is the <body>.

Comment 22

[Boris Zbarsky [:bzbarsky]]

In other words, that's the optimization discussed in comment 2 at the end.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: This fixes the voodooextreme regression on tbox

Still doesn't get us all the way down to where we used to be, but I'll look into that tomorrow.  I did a test checkin and backout of this to verify, since I couldn't reproduce locally (too much noise; the 10% got swallowed by it).

Comment 24

[Boris Zbarsky [:bzbarsky]]

Checked in the followup. Marking fixed, but I'll still look at the numbers tomorrow morning if they don't drift back into the 268 range (which they're sort of working on doing).

Comment 25

[Boris Zbarsky [:bzbarsky]]

Looks like the numbers have ended up where they should, pretty much.

Comment 26

[Jesse Ruderman]

> -  if (gNoisyContentUpdates) {
> +  if (gNoisyContentUpdates || 1) {

Was that change intentional?  I'm seeing the associated printf output a bunch of times when I load Slashdot.  (Mac trunk debug with a few IB-related patches.)

Comment 27

[Boris Zbarsky [:bzbarsky]]

Uh, no.  Removed.