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)
Core
Layout: Text and Fonts
Tracking
()
VERIFIED
FIXED
People
(Reporter: martijn.martijn, Assigned: jfkthame)
Details
(Keywords: perf, testcase)
Attachments
(2 files)
696 bytes,
text/html
|
Details | |
1.24 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
See testcase, this testcase is really slow in rendering for some reason.
Comment 1•13 years ago
|
||
88% of rendering is spent in reverse_range() in harfbuzz/src/hb-buffer.cc
Assignee | ||
Comment 2•13 years ago
|
||
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.
Comment 3•13 years ago
|
||
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?
Comment 4•13 years ago
|
||
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.
Comment 5•13 years ago
|
||
(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)
Comment 6•13 years ago
|
||
I think the inconsistent performance between first reload and refresh is probably due to textrun caching.
Assignee | ||
Comment 7•13 years ago
|
||
(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).
Assignee | ||
Comment 8•13 years ago
|
||
(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.
Assignee | ||
Comment 9•13 years ago
|
||
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.
Assignee | ||
Updated•13 years ago
|
OS: Windows 7 → All
Hardware: x86 → All
Comment 10•13 years ago
|
||
I couldn't reproduce with RTL text and LTR override. Can someone profile how many times we're calling hb_shape() at least?
Assignee | ||
Comment 11•13 years ago
|
||
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)
Comment 12•13 years ago
|
||
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?
Assignee | ||
Comment 13•13 years ago
|
||
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+
Assignee | ||
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 14•13 years ago
|
||
(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.
Comment 15•13 years ago
|
||
Ok. Now that hb has a test suite, I'll write a test that fails first and commit the fix.
Comment 16•13 years ago
|
||
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.
Reporter | ||
Comment 17•13 years ago
|
||
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.
Description
•