67495 - poor perf during incremental reflow in a text control (inline frame reflow problem)

Status: RESOLVED DUPLICATE of bug 67235

(Core :: Layout, defect, P2)

Description

[buster]

To reproduce:
Turn on paint flashing.
Load http://bugzilla.mozilla.org
Type into the text control
You'll see content outside the text control getting repainted.  This is because
it's getting reflowed, and it shouldn't.

See nsBlockFrame::DoReflowInlineFrames()
(http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsBlockFrame.cpp#4246)

At the point where we're looping to reflow each frame on the line, we should
know if we're doing an incremental reflow, and whether that incr. reflow is
constrained (as in the case of a text control, where incr. reflows within the
text control can't change the dimensions of the text control itself.)
If so, we should only reflow |frame| if it is in the path to the target or the
target itself.
For all other frames on the line, |aLineLayout| and maybe |aState|, will have to
recover the state for |frame| as if it was reflowed.  Something like
nsBlockReflowState::RecoverStateFrom() would have to be added to nsLineLayout so
that the line was properly computed.

Comment 1

[buster]

Kevin: this is the remaining "leaking incremental reflow" problem you've been
seeing.

Comment 2

[buster]

Attachment: test case that shows how bad this can be...no table, just a text control followed by a lot of text in the body

Comment 3

[kinmoz]

Cc'ing [e-mail] since he's interested in typing performance in the
browser.

Comment 4

[Chris Waterson]

This could likely be fixed by fixing bug 43914. I tried at one point, and 
failed, causing a regression (bug 56102). I'll revisit...

Comment 5

[Chris McAfee]

perf radar

Comment 6

[Marc Attinasi]

Assigning milestone

Comment 7

[buster]

Attachment: patch that fixes a large percentage of the unnecessary reflow

Comment 8

[buster]

The patch I just attached fixes the majority of this problem.  A better solution
will include expanding the notion of IsIncrementalDamageConstrained() to include
lots of incremental reflow types, not just typing in a text control.

Comment 9

[Marc Attinasi]

Milestone shift --> Moz 0.8.1

Comment 10

[Marc Attinasi]

I've been running with this in since mid February (when buster left) - I just
forgot about it. Maybe you can review it, Chris K. or Chris W.? I'd like to
check it in and take the performance gains if we can.

Comment 11

[Chris Waterson]

Do me a favor and post ``diff -wu'' (because I'm lazy and don't want to have to
apply the patch, but want to see if anything but indentation changed.)

Also, sfraser: is it the case that only plaintext controls have their frame type
as nsLayoutAtoms::textInputFrame? (This is how
nsBlockFrame::IsIncrementalDamangeConstrained() decides whether or not to return
``true''.)

Comment 12

[Marc Attinasi]

Attachment: patch created with -wu (much easier to read)

Comment 13

[Simon Fraser [no longer active]]

waterson: i know not the answer to your question

Comment 14

[Marc Attinasi]

The only references I see to textInputFrame are in layout (where we test it) and
in the GfxTextControlFrame where the frame type is set.

I checked this query:
http://lxr.mozilla.org/seamonkey/search?string=textInputFrame

Seems safe to me...

Comment 15

[Chris Waterson]

Ok, I'm looking at nsGfxTextControlFrame2.cpp, which is what I think is used for
text controls. It's not clear to me whether or not nsGfxTextControlFrame2 is
used for HTML, but I'm pretty sure it's *not*. (Maybe that's the XUL <text> object?)

That said, it looks like nsGfxTextControlFrame2 is used for multi-line text
fields, and I'm a bit worried that these changes might cause weird bustage with
those. Have you made sure that typing in MLE's still works properly? (Especially
with insertion points in the middle of the text, etc., since this affects
pushing and pulling of lines.)

Also, it wouldn't hurt to verify that plaintext mail compose still works right,
as well as doing some sanity checks in the HTML mail compose and editor windows.
If all those check out, r=waterson

Comment 16

[Simon Fraser [no longer active]]

nsGfxTextControlFrame2 is used for single and multi-line text inputs. It is NOT 
used for composer, or plain text compose (which use full-blown composers).

Comment 17

[Marc Attinasi]

OK - I'll do some focused testing on editing single and multiline edit controls.
I have been running with this for a while, filing bugs and the like, but I think
it is only prudent to specifically test the cases you mentioned Chris. Thanks
for the tips and the review.

Comment 18

[Marc Attinasi]

Well, the patch makes editing large blocks of text in multiline edits _much_
faster, but it does have some problems. I pasted some large blocks of text into
a MLE (this one, in fact) and tried prepending, inserting, and appending text.
Each case seemed fine for a while but ended up with text not showing up, lines
getting glommed together or other visual problems. This one will take some
investigating as I am pretty new to the line/block reflow world. 

Pushing out to Mozilla 1.0 to give us lots of time to understand where the
problems are. Thanks, Chris, for the testing clue.

Comment 19

[Tomi Leppikangas]

You guys propably know this already, but easy way to see this bug is to
turn on user_pref("layout.reflow.showframecounts", true);
This is also on prefs panel: Edit->Preferences->Debug->Events->"Show frame
counts"

When you type, only count inside input frame should increment, others
should not change, right?

Comment 20

[Marc Attinasi]

--> p2

Comment 21

[rbs]

Wow, this Edit->Preferences->Debug->Events->"Show frame counts" is great...

Comment 22

[Kevin McCluskey (gone)]

*** This bug has been marked as a duplicate of 67235 ***