465928 - Avoid SetInvalidateTextRuns in nsBlockFrame::DoRemoveFrame

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

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

Attachment: fix

When a frame is complete and we delete its unnecessary (now empty) continuations, we don't need to call SetInvalidateTextRuns on the lines containing those continuations, because we're not removing any actual content. Fixing this requires propagating a flag through DeleteNextInFlowChild to indicate whether we're in that common case. This lets us reduce the amount of textrun reconstruction we do.

Comment 1

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

Comment on attachment 349167 [details] [diff] [review]
fix

> nsBlockFrame::DoRemoveFrame(nsIFrame* aDeletedFrame, PRBool aDestroyFrames,

>         static_cast<nsContainerFrame*>(nif->GetParent())
>-          ->nsContainerFrame::DeleteNextInFlowChild(presContext, nif);
>+          ->nsContainerFrame::DeleteNextInFlowChild(presContext, nif, aRemovingEmptyFrames);

you should wrap to another line rather than cross the 80th column

> nsBlockFrame::DeleteNextInFlowChild(nsPresContext* aPresContext,

>+    nsContainerFrame::DeleteNextInFlowChild(aPresContext, aNextInFlow, aDeletingEmptyFrames);

Same here

>+    * @param aRemovingEmptyFrames all frames we're removing are empty
>+    * so lines do not need to be marked dirty

Do you mean text runs rather than lines?  Or is there something else here?

>   nsresult DoRemoveFrame(nsIFrame* aDeletedFrame, PRBool aDestroyFrames = PR_TRUE, 
>-                         PRBool aRemoveOnlyFluidContinuations = PR_TRUE);
>+                         PRBool aRemoveOnlyFluidContinuations = PR_TRUE,
>+                         PRBool aRemovingEmptyFrames = PR_FALSE);

Having three default parameters, two of which are true and one of which is false, seems very ugly and a bit dangerous.  How many callers depend on the default parameters?  Could the defaults be dropped?  Is it possible to rename your new one such that the default could be true (e.g., to aInvalidateTextRuns)?  (Do you think that would improve things?)


r+sr=dbaron

Comment 2

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

(In reply to comment #1)
> (From update of attachment 349167 [details] [diff] [review])
> > nsBlockFrame::DoRemoveFrame(nsIFrame* aDeletedFrame, PRBool aDestroyFrames,
> 
> >         static_cast<nsContainerFrame*>(nif->GetParent())
> >-          ->nsContainerFrame::DeleteNextInFlowChild(presContext, nif);
> >+          ->nsContainerFrame::DeleteNextInFlowChild(presContext, nif, aRemovingEmptyFrames);
> 
> you should wrap to another line rather than cross the 80th column
> 
> > nsBlockFrame::DeleteNextInFlowChild(nsPresContext* aPresContext,
> 
> >+    nsContainerFrame::DeleteNextInFlowChild(aPresContext, aNextInFlow, aDeletingEmptyFrames);
> 
> Same here

OK

> >+    * @param aRemovingEmptyFrames all frames we're removing are empty
> >+    * so lines do not need to be marked dirty
> 
> Do you mean text runs rather than lines?  Or is there something else here?

I guess just "text runs do not need to be invalidated".

> >   nsresult DoRemoveFrame(nsIFrame* aDeletedFrame, PRBool aDestroyFrames = PR_TRUE, 
> >-                         PRBool aRemoveOnlyFluidContinuations = PR_TRUE);
> >+                         PRBool aRemoveOnlyFluidContinuations = PR_TRUE,
> >+                         PRBool aRemovingEmptyFrames = PR_FALSE);
> 
> Having three default parameters, two of which are true and one of which is
> false, seems very ugly and a bit dangerous.  How many callers depend on the
> default parameters?  Could the defaults be dropped?  Is it possible to rename
> your new one such that the default could be true (e.g., to
> aInvalidateTextRuns)?  (Do you think that would improve things?)

Would it be better if I made them into a single flags word?

Comment 3

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

(In reply to comment #2)
> Would it be better if I made them into a single flags word?

It might, if you think that would come out cleanly (which it might, especially if the flag were always the uncommon case).  I'm mainly concerned about (a) readability and (b) the risk of bugs caused by omitting one of the parameters or getting them in the wrong order.

Comment 4

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

I'll do that then.

Comment 5

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

Attachment: with flags

Comment 6

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

Comment on attachment 349550 [details] [diff] [review]
with flags

>+   * PRESERVE_REMOVED_FRAMES does NOT work on out of flow frames so
>+   * always use PR_TRUE for out of flows.

s/always use PR_TRUE/it should not be passed/ or something like that

r+sr=dbaron

Comment 7

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

Needed for blocker bug 430332.

Comment 8

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

Pushed to trunk as fde162441213 and 2ad0cb394376

Comment 9

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

Comment on attachment 349550 [details] [diff] [review]
with flags

Avoids textrun reconstruction; performance win on bug 430332

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 349550 [details] [diff] [review]
with flags

a191=beltzner

Comment 11

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8c8b403e4094
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/67c5b542ea9e

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 349550 [details] [diff] [review]
with flags

>+   * PRESERVE_REMOVED_FRAMES does NOT work on out of flow frames so
[snip]
>+  enum {
>+    PERSERVE_REMOVED_FRAMES    = 0x01,

I just ran across this typo when perusing nsBlockFrame.cpp ("pERserve instead of pREserve in the enum value name).  It's spelled correctly in all the comments, and incorrectly in all the code.

Looks like this file just needs a s/PERSERVE/PRESERVE/.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: followup typo fix: s/PERSERVE/PRESERVE/ in nsBlockFrame.*

Comment 14

[Daniel Holbert [:dholbert]]

Followup typo fix landed on mozilla-central and 191 branch:
http://hg.mozilla.org/mozilla-central/rev/9daec2349061
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/703fdc2d941e