Closed Bug 332655 Opened 15 years ago Closed 11 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: smontagu, Assigned: smontagu)

References

(Blocks 1 open bug)

Details

(Keywords: fixed1.9.1, perf)

Attachments

(2 files, 4 obsolete files)

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.
Attached 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.
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.
... 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.
Attachment #217127 - Attachment is obsolete: true
Attachment #217127 - Flags: review?(uriber)
Attached 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.
Attached 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.
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: layout.bidi → layout.fonts-and-text
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?
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?
Blocks: 477495
Keywords: perf
Attached 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
Attached 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?
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?
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
Blocks: 423264
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?
(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.
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
Summary: Some code cleanup in nsBidiPresUtils → Don't join up text frames with the same content in Bidi resolution
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+
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
Blocks: 487724
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.
http://hg.mozilla.org/mozilla-central/rev/f9627950be8f
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Depends on: 489691
Depends on: 489887
Depends on: 490747
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.
Depends on: 491547
Depends on: 492231
Depends on: 496006
Blocks: 455643
Depends on: 499538
Depends on: 536963
You need to log in before you can comment on or make changes to this bug.