161825 - GetWidth optimizations need to be implemented on BeOS (text measurement performance)

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P5)

Description

[Roland Mainz]

[follow-up of bug 36146 ("GetWidth optimizations need to be implemented on Unix
(text measurement performance)")]

RFE: GetWidth optimizations need to be implemented on BeOS (text measurement
performance)

Comment 1

[Sergei Dolgov]

Will look at problem.
Though, a notice. Text rendering in BeOS port may be much faster if
nsRendereningContext methods will be called with already UTF8 strings instead
PRUnichar *.

Comment 2

[Sergei Dolgov]

Seems stubs are too simplicistic or layout code ingores fact of missing
GetTextDimensions  - it causes very bad rendering of such pages as
http://www.aftonbladet.se/vss/noje/story/0,2789,259021,00.html

Comment 3

[Sergei Dolgov]

http://www.unisys.com - right column
http://www.submarino.com.br - left column

Comment 4

[Sergei Dolgov]

advanced GetTextDimensions tends to produce weird results if 198890 isn't fixed

Comment 5

[Sergei Dolgov]

wondering if mAveCharWidth = width_of "x" is good approach for non-latin text

Comment 6

[Sergei Dolgov]

Attachment: Patch. First attempt.

Ok, here is simple patch. Hope i didn't forget something.

Paul, anyway, i'm unable to test it now (and next 10 day) with plain CVS
sources.
Hope you do.

patch needs rebuild of mozilla/gfx,

make clean/make in 
mozilla/layout/html/base/src
and then make in
mozilla/layout/build.

Comment 7

[Sergei Dolgov]

Adding rds (Roger B. Sidje) to CC as main authority

Comment 8

[Paul]

Comment on attachment 120709 [details] [diff] [review]
Patch. First attempt.

>+// Implementation is simplicistic in comparison with other platforms - we follow in this mehod

Misspelled "method" (I know, nitpicking :-)

>+		utf8buf = (uint8 *)mSurface->GetCharArray();

nsDrawingSurfaceBeOS does not define GetCharArray, or at least my version
doesn't.

Also, it seems your indenting with both tabs and spaces ;-)

Comment 9

[Sergei Dolgov]

Attachment: patch. 2nd take.

Removed yet existent mSurface storage call, revised identation and spelling,
added contributor name in nsRenderingContextbeOS.h

Comment 10

[Roland Mainz]

(from attachment 120871 [details] [diff] [review]:)
> NS_IMETHODIMP nsRenderingContextBeOS::GetHints(PRUint32 &aResult) {
>        aResult = 0;
>+       aResult |= NS_RENDERING_HINT_FAST_MEASURE;      
>        return NS_OK;
> }

I strongly suggest to add a env variable check or a prefs to turn this
"on"/"off" on demand.
In gfx/src/gtk/||gfx/src/xlib/ this helped much to hunt regressions/problems on
the user's side without having the user to build mozilla first (or sending debug
builds around) in the past and I assume this will help on BeOS as well...

Comment 11

[Sergei Dolgov]

2 Roland.
Yeah, i probably will add it after Paul has current patch tested  - if it
applies and works at all.
But i prefer in our case to add env variable dependency in way, where
FAST_MEASURE is ON by default, and setting variable switches it off.
As we haven't special testers, and hardly we can find sufficient number of
enthousiast to set some suspicious variable to test some feature. And also BeOS
is not Linux in some sense - there is no such desire (and need!)here to do
something in CLI or edit text configs :))). So if someone report some weirdness,
i will recommend her/him to set variable. But not vice versa.

Comment 12

[Paul]

Ok, I applied the updated patch.  Did a full rebuild, and, it seems to work just
fine.  No problems here.  And Sergei, I agree with your comment.  Yes, we should
have the option of turning this off, but, by default, I feel it should be turned
on, as, like you said, most users will more than likely not turn it on
themselves, and therefor, it would not get tested.

Comment 13

[Sergei Dolgov]

Paul, do you have some time to add this option, se we can check it in before
tree is closed? I doubt that i can do it myself in next 2 or 3 days:(

Comment 14

[Paul]

Attachment: above patch, with ability to turn off changes

Added a check for an environment variable.  If set, changes will not be used.
Need sr= for changes to layout code

Also, this patch makes me wonder, are the any other optimization methods within
mozilla that BeOS has be "#ifdef"'d out of?

Comment 15

[rbs]

Comment on attachment 121015 [details]
above patch, with ability to turn off changes

sr=rbs, assuming you folks are happy with those details specific to BeOS.

Comment 16

[Paul]

Sergei, if you're ok with everything i'll check this in ASAPr=arougthopher

Comment 17

[Sergei Dolgov]

Seems OK for me.
I'm wondering if we can do some profiling/benchmarking with this option ON and
OFF on some complex pages with lot of tables/text frames with justify alignment.
On other platforms IIRC 5-20% preformance increase was reported.

Comment 18

[Paul]

Though I agree some profiling would be nice, I did see a noticable difference in
rendering certain pages, when I added the env check, and having this turned ON
or OFF.

Marking fixed, as I have checked this in.