Closed Bug 390425 Opened 13 years ago Closed 12 years ago

[FIXr]Simplify {ib} handling a bit

Categories

(Core :: Layout, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(2 files, 4 obsolete files)

Attached patch Fix (obsolete) — Splinter Review
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?
Attachment #274728 - Flags: superreview?(roc)
Attachment #274728 - Flags: review?(roc)
Attached patch Updated to tip (obsolete) — Splinter Review
Attachment #274728 - Attachment is obsolete: true
Attachment #275037 - Flags: superreview?(roc)
Attachment #275037 - Flags: review?(roc)
Attachment #274728 - Flags: superreview?(roc)
Attachment #274728 - Flags: review?(roc)
Blocks: 390976
+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?
> 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...
> 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?
BTW does your patch fix bug 391628 by any chance?
> 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.
Blocks: 391628
> 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.
Attached patch Updated to comments (obsolete) — Splinter Review
Attachment #275037 - Attachment is obsolete: true
Attachment #276085 - Flags: superreview?(roc)
Attachment #276085 - Flags: review?(roc)
Attachment #275037 - Flags: superreview?(roc)
Attachment #275037 - Flags: review?(roc)
Comment on attachment 276085 [details] [diff] [review]
Updated to comments

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

!aFrame->GetPrevContinuation() would be simpler/faster?
Attachment #276085 - Flags: superreview?(roc)
Attachment #276085 - Flags: superreview+
Attachment #276085 - Flags: review?(roc)
Attachment #276085 - Flags: review+
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.
Attachment #276085 - Flags: approval1.9?
Summary: [FIX]Simplify {ib} handling a bit → [FIXr]Simplify {ib} handling a bit
Comment on attachment 276085 [details] [diff] [review]
Updated to comments

A bit scary, but a1.9=dbaron.
Attachment #276085 - Flags: approval1.9? → approval1.9+
Checked in.
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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.
Attached patch Patch to put this back in. (obsolete) — Splinter Review
I backed this out to fix Tp
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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.
Attachment #276085 - Attachment is obsolete: true
Attachment #276197 - Attachment is obsolete: true
Attachment #276399 - Flags: superreview?(roc)
Attachment #276399 - Flags: review?(roc)
Note: the diff against the "patch to put this back in" indicates which parts really need review...
Blocks: 386642
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));
Attachment #276399 - Flags: superreview?(roc)
Attachment #276399 - Flags: superreview+
Attachment #276399 - Flags: review?(roc)
Attachment #276399 - Flags: review+
Attachment #276399 - Flags: approval1.9+
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.
To be more clear, the tinderbox regression is Linux-only.
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>.
In other words, that's the optimization discussed in comment 2 at the end.
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).
Attachment #276892 - Flags: superreview?(roc)
Attachment #276892 - Flags: review?(roc)
Attachment #276892 - Flags: superreview?(roc)
Attachment #276892 - Flags: superreview+
Attachment #276892 - Flags: review?(roc)
Attachment #276892 - Flags: review+
Attachment #276892 - Flags: approval1.9+
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).
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Looks like the numbers have ended up where they should, pretty much.
Depends on: 393517
Depends on: 393649
Depends on: 393661
> -  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.)
Uh, no.  Removed.
Depends on: 393671
Depends on: 394111
No longer depends on: 468768
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.