646359 - Optimize bidi resolution on lines with single-direction text

Status: RESOLVED FIXED

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

Description

[Simon Montagu :smontagu]

I originally included a patch to do this in attachment 495873 [details] [diff] [review] as part of bug 263359 but it caused bug 654419.

It's a very significant performance win in large files where most lines are left-to-right but there are a few lines containing right-to-left text -- tinderbox logs are the classic example for developers.

Comment 1

[Simon Montagu :smontagu]

Attachment: Patch

Bug 645119 was caused by only dirtying the line when the directionality of the frame had changed.

Comment 2

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

Comment on attachment 523273 [details] [diff] [review]
Patch

Sorry about the delay.

Is there a way to not always mark the line dirty? What is the case in bug 645119 where the line needs to be marked dirty?

Comment 3

[Simon Montagu :smontagu]

What is the cost of marking the line dirty? I think that to prevent bug 645119 all we need to do is repaint the line, so if there is a cheaper way to do that it would be sufficient.

Comment 4

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

Marking the line dirty means it will have to be reflowed. Possibly other lines next to it will be reflowed as well.

You can invalidate the line's "combined area" if you want.

Comment 5

[Jet Villegas (inactive)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> 
> r+

Hey, a reviewed patch from 2011! What does it need to land?

Comment 6

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

I think Simon just needs to call SchedulePaint now to ensure that we re-run DLBI and paint if necessary.

Comment 7

[Simon Montagu :smontagu]

All my attempts to call SchedulePaint (or InvalidateFrame or InvalidateFrameSubtree) end up leaving text turds on the screen in the scenario from bug 645119. I'm guessing that this is because when entering or deleting a newline in a text area and making existing lines scroll up or down, the area that gets invalidated is where the line *used* to be, and it doesn't get painted in its new position.

Comment 8

[Simon Montagu :smontagu]

Attachment: Simplified testcase for bug 645199

STR: focus the caret at the top of the textarea and press Enter

Comment 9

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

(In reply to Simon Montagu :smontagu from comment #7)
> All my attempts to call SchedulePaint (or InvalidateFrame or
> InvalidateFrameSubtree) end up leaving text turds on the screen in the
> scenario from bug 645119. I'm guessing that this is because when entering or
> deleting a newline in a text area and making existing lines scroll up or
> down, the area that gets invalidated is where the line *used* to be, and it
> doesn't get painted in its new position.

No, that sort of thing doesn't happen anymore with DLBI.

DLBI doesn't handle text frames being displayed with different contents, so SchedulePaint isn't enough given we're changing the way a textframe renders --- but InvalidateFrame or InvalidateFrameSubtree on the text frame should still work. I don't know why it doesn't...

Comment 10

[Simon Montagu :smontagu]

There's something I don't understand here, and maybe explaining that would indicate a way forward: if I hack the code so that we don't do bidi resolution at all, there is no regression in the testcase in comment 8.

What is bidi doing that prevents the lines repainting properly?

Comment 11

[Simon Montagu :smontagu]

As so often, 90% of the solution was asking the right question: since bug 263359 bidi can create continuation frames after hard line breaks while walking the frames preparing for bidi resolution. When this happens we need to reflow the line just as we do after creating non-fluid continuations during bidi resolution itself.

Try run at https://tbpl.mozilla.org/?tree=Try&rev=6cfaabfdde8d has some remaining issues which I'm fixing.

Comment 12

[Simon Montagu :smontagu]

Attachment: Updated patch

With the patch for bug 936935, there don't seem to be any regressions any more.

Comment 13

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdf16bdfce5a

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bdf16bdfce5a