bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Reduce number of calls to nsLineBox::Contains from nsBidiPresUtils::Resolve

RESOLVED FIXED

Status

()

Core
Layout: Text
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: Uri Bernstein (Google), Assigned: Uri Bernstein (Google))

Tracking

({perf})

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

12 years ago
This bug is for implementing the performance optimization suggested by BZ in bug 330373 comment #24:

> It seems to me that we should be doing the advancing at the places where we
> call MarkDirty(); perhaps with a function that takes the frame,
> nsBlockFrame::line_iterator& line, const nsBlockFrame::line_iterator& endLines?
>  That would also address the second concern I had, which is that the code as it
> stands always iterates the lines, even if it doesn't actually change any
> continuations.

The performance problem is significant in lines with very many frames on them. I noticed this when trying to do "view selected source" after selecting the talkback section in http://www.ynet.co.il/articles/0,7340,L-3256726,00.html. The source contains about 1MB of complex HTML on a single line, which takes forever to render (effectively, the browser hangs). Using the "sample" feature in OS X's Activity Monitor, I can see that much of the time is spent in nsLineBox::IndexOf, called (via nsLineBox::Contains), from nsBidiPresUtils::Resolve.
(Assignee)

Comment 1

12 years ago
Created attachment 223803 [details] [diff] [review]
patch
Assignee: nobody → uriber
Status: NEW → ASSIGNED
Attachment #223803 - Flags: superreview?(bzbarsky)
Attachment #223803 - Flags: review?(bzbarsky)
Comment on attachment 223803 [details] [diff] [review]
patch

What about the concern you had in bug 330373 comment 24?

r+sr=bzbarsky if you've resolved that to your satisfaction.
Attachment #223803 - Flags: superreview?(bzbarsky)
Attachment #223803 - Flags: superreview+
Attachment #223803 - Flags: review?(bzbarsky)
Attachment #223803 - Flags: review+
(Assignee)

Comment 3

12 years ago
Hmmm... thanks for reminding me of that.

It turns out that the newly-created frames do "know" what line they are on (the frame count on the line is incremented when creating a new continuation), so there is no functional problem.

However, this change does cause nsLineBox::Contains to be called in cases where it currently isn't called (that is, for newly-created continuations), so I assume that for some cases (where there is originally a small number of frames, but some of them are broken into many bidi continuations), this will actually cause a performance hit.

As the case described above is possibly more common than the one described in comment #0, I'm now hesitating whether this is worth doing.
(Assignee)

Comment 4

12 years ago
Created attachment 223815 [details] [diff] [review]
patch v2

This should give us the best of both worlds.
Attachment #223803 - Attachment is obsolete: true
Attachment #223815 - Flags: review?(bzbarsky)
Comment on attachment 223815 [details] [diff] [review]
patch v2

Yeah, I like this!
Attachment #223815 - Flags: superreview+
Attachment #223815 - Flags: review?(bzbarsky)
Attachment #223815 - Flags: review+
(Assignee)

Comment 6

12 years ago
Checking in layout/base/nsBidiPresUtils.cpp;
/cvsroot/mozilla/layout/base/nsBidiPresUtils.cpp,v  <--  nsBidiPresUtils.cpp
new revision: 1.77; previous revision: 1.76
done
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

10 years ago
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: layout.bidi → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.