Don't join up text frames with the same content in Bidi resolution

RESOLVED FIXED

Status

()

P2
normal
RESOLVED FIXED
13 years ago
9 years ago

People

(Reporter: smontagu, Assigned: smontagu)

Tracking

(Blocks: 1 bug, {fixed1.9.1, perf})

Trunk
fixed1.9.1, perf
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

13 years ago
This is really part of the work for bug 263359, but I want to fix it separately since it applies in all cases and I don't want to make the patch there more complicated than it already is.
(Assignee)

Comment 1

13 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #217127 - Flags: review?(uriber)
This doesn't work for me, because on the initial load of the page, when we get to Reslove(), mContentLength of the text frames is not yet set correctly (it is simply initialized to "0"). So CreateBlockBuffer() returns a zero-length buffer, and things go downhill from there. The end result is that no bidi resolving happens, and RTL portions of an LTR paragraph appear reversed.
... this is because in nsBlockFrame::Reflow(), bidiUtils->Resolve() is called before calling ReflowDirtyLines() (which, eventually, assigns values to mContentLength of descendant text frames).

I was about to suggest delaying the call to Resolve() until after ReflowDirtyLines(), but that won't work because bidi frame-ordering also happens under ReflowDirtyLines(), and depends on Resolve() to have already been executed.
(Assignee)

Comment 4

13 years ago
It's odd that it worked for me, then. I have been juggling different patches, so I might not have been testing what I thought I was.
(Assignee)

Comment 5

13 years ago
... on the other hand, I was testing what I thought I was at least some of the time, because there were initial errors which I fixed in a second iteration. What seems more likely is that it works in larger files where incremental reflow kicks in but not in smaller files.
(Assignee)

Updated

13 years ago
Attachment #217127 - Attachment is obsolete: true
Attachment #217127 - Flags: review?(uriber)
(Assignee)

Comment 6

13 years ago
Posted patch patch v.2 (obsolete) — Splinter Review
This patch no longer has that problem. Whether it achieves the stated aim of simplifying the code is open to question.
Assignee: nobody → smontagu
Status: NEW → ASSIGNED
(In reply to comment #6)
> This patch no longer has that problem. Whether it achieves the stated aim of
> simplifying the code is open to question.
> 

Even if it doesn't achieve that aim, I expect it to greatly improve performance, as (upon non-initial reflows) the first in-flow will no longer be gobbling up the content from all its continuations, and thus line-breaking code won't have to do the line breaking all over again (which I think is the main cause of bug 330350).

I'll do some more testing now.
Posted file useful testcase
This is a simple testcase which was useful to me when I was working on this code, and I'm now using it when testing the patches. Clicking on either of the links causes a reflow of that link.

With patch v2 applied, clicking on the top link causes the LTR text to be reversed, and clicking on the bottom link causes the RTL word (at the end) to be reversed.

I haven't diagnosed the cause of this, yet.
So, what happens is that after processing the first (RTL) frame, the condition for calling RemoveBidiContinuation() is satisfied, and that call "removes" (actually, turns into fluid continuations) the rest of the frames.
We probably want to make sure we're calling RemoveBidiContinuation only after processing the entire content node, or something.

Updated

11 years ago
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: layout.bidi → layout.fonts-and-text

Comment 10

10 years ago
Hi,

I have noticed multiple regression with respect to Bidi reordering.

There is noticeable refactoring to the nsBidiPresUtils code I originally wrote around year 2000. But where can one find a description for significant changes if any?
(Assignee)

Comment 11

10 years ago
You can find a list of all changes at http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/layout/base/nsBidiPresUtils.cpp&rev=HEAD (up to the 1.9.0 branch) and http://hg.mozilla.org/mozilla-central/log/b42b8de951c1/layout/base/nsBidiPresUtils.cpp (for current trunk).

Can you file bugs for the regressions that you are seeing?
(Assignee)

Updated

10 years ago
Blocks: 477495
(Assignee)

Updated

10 years ago
Keywords: perf
(Assignee)

Comment 12

10 years ago
Posted patch Patch v.3 (buggy) (obsolete) — Splinter Review
This version doesn't regress the testcase in attachment 217143 [details], but it does regress layout/reftests/bidi/with-first-letter-2a.html and with-first-letter-2b.html. It reduces the wall-clock time of loading a local copy of http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1234111896.1234114660.22461.gz from about 38 minutes to about 6 minutes.
Attachment #217142 - Attachment is obsolete: true
(Assignee)

Comment 13

10 years ago
Posted patch Patch v.4 (still buggy) (obsolete) — Splinter Review
Attaching this as checkpoint. It passes reftests, crashtests, and mochitests, and even removes a bunch of assertions in crashtests, but it still has problems with bidirectional text entry.

Load time for a local copy of a tinderbox log with this patch is about 1.5 minutes.
Attachment #363760 - Attachment is obsolete: true
(In reply to comment #13)
> Attaching this as checkpoint. It passes reftests, crashtests, and mochitests,
> and even removes a bunch of assertions in crashtests, but it still has problems
> with bidirectional text entry.

Can you add a mochitest for bidirectional text entry that would fail because of these problems?
(Assignee)

Comment 15

10 years ago
What I really need is a combined mochitest and reftest, something like:
 simulate entering characters
 capture reference rendering
 simulate entering character
 simulate backspace
 capture test rendering
 compare renderings
Is that possible?
(Assignee)

Comment 17

10 years ago
This is looking good to me, but I'll dogfood with it for a day or two before asking review. Comments welcome in the meanwhile. Uri, do you have any more suggestions for testing?

I've also pushed it to the try server in http://hg.mozilla.org/try/rev/b36e6e63814a
Attachment #364747 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Blocks: 423264
(Assignee)

Updated

10 years ago
Blocks: 471619
This looks almost too good to be true...
Doesn't removing the re-use of existing continuation cause excessive frame creation and deletion, which, in turb, causes a performance regression? Have you tried profiling this on large, massively-bidi, text?
(Assignee)

Comment 19

10 years ago
(In reply to comment #18)
> This looks almost too good to be true...

I don't think I've ever been called that before ;-)

> Doesn't removing the re-use of existing continuation cause excessive frame
> creation and deletion, which, in turb, causes a performance regression? Have
> you tried profiling this on large, massively-bidi, text?

No, it doesn't seem to. I think that's because we are resynchronizing with the existing frames with each new frame that we process, rather than with each content node as we used to do. Note also that we still don't delete frames in RemoveBidiContinuation.

I've been using the testcase from bug 319930 (attachment 206580 [details]) as an example of large, fairly massively-bidi, text, and it is about 4 times faster with this patch.
(Assignee)

Comment 20

10 years ago
It's especially interesting to compare load times of files of different lengths derived from attachment 206580 [details]:

Lines in file   Load time without the patch Load time with the patch
 32000           13                          5
 64000           56                          9
128000          182                         23
(Assignee)

Updated

10 years ago
Summary: Some code cleanup in nsBidiPresUtils → Don't join up text frames with the same content in Bidi resolution
(Assignee)

Comment 21

10 years ago
Comment on attachment 364999 [details] [diff] [review]
patch v.5 (possibly not buggy)

This is still looking good; time to request reviews.
Attachment #364999 - Flags: superreview?(roc)
Attachment #364999 - Flags: review?(uriber)
Comment on attachment 364999 [details] [diff] [review]
patch v.5 (possibly not buggy)

rs=me
Attachment #364999 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 23

10 years ago
Mochitests for the editing issues I encountered pre-checked-in in http://hg.mozilla.org/mozilla-central/rev/b7b8d7526192
Requesting blocking1.9.1 for the sake of bug 477495 (so we don't need to wait for approval1.9.1).
Flags: blocking1.9.1?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
(Assignee)

Updated

10 years ago
Blocks: 487724

Comment 25

10 years ago
Any progress here?
Attachment #364999 - Flags: review?(uriber) → review+
Comment on attachment 364999 [details] [diff] [review]
patch v.5 (possibly not buggy)

r=me. Sorry for the very long delay.

The only thing I'd suggest is to inline EnsureBidiContinuation, as its very sort now, called from one place, and doesn't really match its name any more.

Also, I wish this code had more comments - but that's at least partially my fault.
Thanks Uri!
Whiteboard: [needs review] → [needs landing]
Simon, we should get this on trunk ASAP. Add 'checkin-needed' if you want me to land it.
(Assignee)

Comment 29

10 years ago
http://hg.mozilla.org/mozilla-central/rev/f9627950be8f
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
(Assignee)

Updated

10 years ago
Depends on: 489691
(Assignee)

Updated

10 years ago
Depends on: 489887
(Assignee)

Updated

10 years ago
Depends on: 490747
(Assignee)

Comment 30

10 years ago
http://hg.mozilla.org/mozilla-central/rev/373f39a2d1ec

Checked in reftests based on attachment 217143 [details], because many of my attempts to fix the regressions from this bug ended up breaking it.
(Assignee)

Updated

10 years ago
Depends on: 491547
(Assignee)

Updated

10 years ago
Depends on: 492231
(Assignee)

Updated

10 years ago
Depends on: 496006
(Assignee)

Updated

10 years ago
Blocks: 455643
(Assignee)

Updated

10 years ago
Depends on: 499538
(Assignee)

Updated

9 years ago
Depends on: 536963
You need to log in before you can comment on or make changes to this bug.