504524 - Don't dirty all textframe continuations when text is inserted

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

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

Currently when text is inserted before the end of a text node, we dirty the textframe containing the insertion and all the following continuations. That's overkill, and if you have a large amount of text in a single textnode being edited, it will be bad for performance.

Comment 1

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

Currently nsTextFrame::CharacterDataChanged loops over text frame continuations marking them with NS_FRAME_IS_DIRTY. That's actually bad, right? Because we're not calling FrameNeedsReflow and the parent frames aren't being marked with NS_FRAME_HAS_DIRTY_CHILDREN. I guess it's "working" because we never skip reflowing the non-dirty children of inline elements so NS_FRAME_HAS_DIRTY_CHILDREN doesn't matter for them. We're not marking the lines dirty either, but things work out because right now whenever we reflow an inline frame that's not complete we mark the next line dirty. Of course I'm going to have to change that here.

Actually marking all affected textframes with FrameNeedsReflow would be pretty expensive though, for a large change in the text ... a large number of calls to MarkIntrinsicWidthsDirty on all ancestors, for example. Maybe an acceptable optimization would be to not call FrameNeedsReflow if the continuation has the same parent as the previous continuation we already called FrameNeedsReflow on.

Comment 2

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

Attachment: testcase

Load this testcase with the HTML5. You'll get 100,000 lines in a single text node. Try inserting and deleting characters at the top, middle, and bottom.

Comment 3

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

Attachment: fix

With this patch I can type or delete characters anywhere in those lines and only a few lines are reflowed. Need to run reftests and do more testing.

Comment 4

[Boris Zbarsky [:bzbarsky]]

It might be nice to add a comment explaining why after a mutation the content offsets are such for all the frames we haven't marked dirty yet that if another mutation comes along we'll mark the right frames dirty....  Plus adding testcases to that effect, of course.

Comment 5

[Simon Montagu :smontagu]

Is this going to be OK with bidi? For example, if you have lots of lines of neutral characters and you insert a right-to-left character at the beginning, all the following characters will need to change direction.

Comment 6

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

If bidi is enabled, then every mutation causes bidi re-resolution to run. Won't that ensure everything is OK?

Comment 7

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

Patch seems to pass reftests as-is.

Comment 8

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

(In reply to comment #4)
> It might be nice to add a comment explaining why after a mutation the content
> offsets are such for all the frames we haven't marked dirty yet that if another
> mutation comes along we'll mark the right frames dirty....  Plus adding
> testcases to that effect, of course.

I'll add tests for that, but I'm not sure why the multiple mutation case needs to be specially commented upon. The offsets for the non-dirty frames have to be updated correctly, otherwise basic things like painting would break.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I guess it wasn't obvious to me on skimming the patch why the offset manipulations performed are the right ones; I'll have to read it more carefully.

Comment 10

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

Ah I see what you mean.

After writing some more tests, I found at least one bug in the offset manipulations. So it shouldn't have been obvious :-).

Comment 11

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

Attachment: fix v2

Ah no, the bug was somewhere else ... we weren't reflowing lines that contain text frames dirtied by CharacterDataChanged, because the lines weren't dirty.

I added some comments to try to help understand what's going on with the character offset changes. And tests.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 389084 [details] [diff] [review]
fix v2

r=bzbarsky with the other SplitLine callsite's MarkDirty() also nixed.

Comment 13

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

Attachment: prepatch

I forgot to request review on this pre-patch that changes the signature of nsIFrame::CharacterDataChanged

Comment 14

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

I checked this in but backed out due to failures in test_bug332655-2.html on Mac and Linux:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1249440415.1249446860.29769.gz#err1

Comment 15

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

But I did leave the CharacterDataChanged patch in:
http://hg.mozilla.org/mozilla-central/rev/acba6709fc4a

Comment 16

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

These are the test failures:
43221 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_bug332655-2.html | deleting and inserting RTL char at beginning of line shouldn't change 
43222 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_bug332655-2.html | the order entering Bidi text shouldn't change rendering

Comment 17

[Simon Montagu :smontagu]

So I applied the patch locally to see if I could help with this, and it turns out that the regression is fixed by my patch for bug 514156. Pushing both patches to tryserver now.

Comment 18

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

Mmm, that would be great! Thanks!

Comment 19

[Simon Montagu :smontagu]

At the third attempt, all tryserver tests passed. Builds are at https://build.mozilla.org/tryserver-builds/[e-mail]-try-1f8a5adc822d/ (The first time I forgot to hg add the reftests and the second time failed because there were errors in the filenames in reftests/text/reftest.list). The push at http://hg.mozilla.org/try/rev/819363eae9b1 also includes a couple of small changes to make the patch apply to current trunk.

Comment 20

[Simon Montagu :smontagu]

Repushed http://hg.mozilla.org/mozilla-central/rev/d3298de30066

Comment 21

[(no longer active)]

This might benefit from bug 240933, so adding a dependency for now.

Comment 22

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

roc: does this block?

Comment 23

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

No. It's a performance improvement and a somewhat risky one.