Closed Bug 669175 Opened 13 years ago Closed 13 years ago

Slow rendering of text sometimes in this case, using direction: rtl

Categories

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

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: jfkthame)

Details

(Keywords: perf, testcase)

Attachments

(2 files)

Attached file testcase
See testcase, this testcase is really slow in rendering for some reason.
88% of rendering is spent in reverse_range() in harfbuzz/src/hb-buffer.cc
What sort of "really slow" are we looking at here? When I tried repeatedly reloading the testcase on my Win7 laptop, it usually reported around 30ms, sometimes more like 50-60ms .... but occasionally it took "forever" (appeared to hang without actually painting the text, and when I eventually touched the window in some way and triggered a refresh, it reported 8000+ milliseconds). The unpredictability seemed a bit worrying....

For comparison, Chrome on the same machine reported times significantly under 20ms.
First time loading, I always get over 10000ms-ish in ff5 on Linux, and CPU is busy.  Subsequent refreshes are between 50 and 200ms.  Is layout cached across refreshes?!  No clue what's going on.

Simon, how did you measure that?
If I remove any of direction: rtl and unicode-bidi: bidi-override, it becomes fast again.  I'm fairly sure it has something to do with gecko layout code shaping text way too many times, reflowing, or something.
(In reply to comment #3)
> First time loading, I always get over 10000ms-ish in ff5 on Linux, and CPU
> is busy.  Subsequent refreshes are between 50 and 200ms.  Is layout cached
> across refreshes?!  No clue what's going on.
> 
> Simon, how did you measure that?

I used jprof (but I only tried to load it once)
I think the inconsistent performance between first reload and refresh is probably due to textrun caching.
(In reply to comment #6)
> I think the inconsistent performance between first reload and refresh is
> probably due to textrun caching.

Yes, I expect so. However, it bothers me that if I keep reloading, I get occasional repeats of the "slow" case - this may indicate that textruns are expiring from the cache when they shouldn't (because they're being repeatedly re-used).
(In reply to comment #4)
> If I remove any of direction: rtl and unicode-bidi: bidi-override, it
> becomes fast again.  I'm fairly sure it has something to do with gecko
> layout code shaping text way too many times, reflowing, or something.

That's possible, though it is interesting to note that if I use the Core Text shaper instead of Harfbuzz, the problem doesn't occur - and Gecko layout/reflow behavior should be the same in both cases, the difference is just at the lowest textrun shaping level.
Also note that the problem does not occur if I use "real" RTL text (e.g. Arabic letters) instead of Latin text with bidi-override. It's the reversal of "non-natural-direction" text within harfbuzz that's costing us so much. Presumably (not tested) the same would occur if we used Arabic or Hebrew text with LTR override.
OS: Windows 7 → All
Hardware: x86 → All
I couldn't reproduce with RTL text and LTR override.

Can someone profile how many times we're calling hb_shape() at least?
The main problem here is actually a bug in reverse_range() - it doesn't use the  start  parameter properly for the  pos  array. (I'm a bit surprised this hasn't manifested itself in incorrect output, at least not anywhere we've noticed - I suppose we don't have particularly complex test cases that use direction-overridden text.)

Behdad, please confirm that this fix is correct. It looks like the bug is still present in harfbuzz-ng trunk code.

(I also modified the function to use pointers instead of array indexing, btw; this gave a significant perf benefit in my debug build, at least. It might well end up equivalent in an opt build.)
Assignee: nobody → jfkthame
Attachment #544042 - Flags: review?(mozilla)
Wow, good catch.  I guess we just never really tested it under such conditions.  Thanks Jonathan!  Looks good.  Can you push to harfbuzz master too, or want me to do that?
Comment on attachment 544042 [details] [diff] [review]
patch, fix reverse_range() in hb-buffer

Taking comment #12 as r=behdad. :)

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/a6dd98edd75f
Attachment #544042 - Flags: review?(mozilla) → review+
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
(In reply to comment #12)
> Can you push to harfbuzz master
> too, or want me to do that?

I'm not currently set up to push there.... probably need to configure keys or something. So feel free to push it for me, or else I'll try to figure out this git stuff a bit later.
Ok.  Now that hb has a test suite, I'll write a test that fails first and commit the fix.
Re the performance boost, I tested with cachegrind, the indexed loop is just a tad bit faster than the pointer one.  Guess gcc just knows how to optimize a simple loop better.
Thanks for fixing! Verified fixed, using the 8.0a1 (2011-07-07) build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: