Optimize bidi resolution on lines with single-direction text

RESOLVED FIXED in Firefox 28

Status

()

Core
Layout: Text
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: smontagu, Assigned: smontagu)

Tracking

(Blocks: 5 bugs, {perf})

Trunk
mozilla28
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox28 fixed)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

7 years ago
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.

Updated

7 years ago
Keywords: perf
(Assignee)

Comment 1

7 years ago
Created attachment 523273 [details] [diff] [review]
Patch

Bug 645119 was caused by only dirtying the line when the directionality of the frame had changed.
Assignee: nobody → smontagu
Attachment #523273 - Flags: review?(roc)
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?
Attachment #523273 - Flags: review?(roc) → review+
(Assignee)

Comment 3

7 years ago
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.
(Assignee)

Updated

7 years ago
Blocks: 650189
(Assignee)

Updated

7 years ago
No longer blocks: 650189
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.
(Assignee)

Updated

6 years ago
Blocks: 722641

Comment 5

5 years ago
(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?
I think Simon just needs to call SchedulePaint now to ensure that we re-run DLBI and paint if necessary.
(Assignee)

Comment 7

5 years ago
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.
(Assignee)

Comment 8

5 years ago
Created attachment 775514 [details]
Simplified testcase for bug 645199

STR: focus the caret at the top of the textarea and press Enter
(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...
(Assignee)

Updated

4 years ago
Blocks: 762710
(Assignee)

Comment 10

4 years ago
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?
(Assignee)

Comment 11

4 years ago
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.
(Assignee)

Updated

4 years ago
Depends on: 936935
(Assignee)

Comment 12

4 years ago
Created attachment 832100 [details] [diff] [review]
Updated patch

With the patch for bug 936935, there don't seem to be any regressions any more.
Attachment #523273 - Attachment is obsolete: true
Attachment #832100 - Flags: review?(roc)
Attachment #832100 - Flags: review?(roc) → review+
(Assignee)

Comment 13

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdf16bdfce5a
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/bdf16bdfce5a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

4 years ago
Depends on: 964281

Updated

4 years ago
Depends on: 965602

Updated

4 years ago
No longer depends on: 965602
status-firefox28: --- → fixed
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.