310708 - [BeOS]Font width cache to be implemented

Status: RESOLVED FIXED

(Core Graveyard :: GFX: BeOS, defect)

Description

[Sergei Dolgov]

It is about last HUGE performance bottleneck in our GFX layer (there are others,
but much less important)

For sites which use specual char alignment (like align=justify) )Mozilla
permanently calculates placement for each char, and to calculate this placement,
it should permanently ask char widths.
But in BeOS each call for String/CharWidth, be it BView method or BFont method,
actually calls app_server, which brings HUGE overhead and slowdown. Like 5000
extra appserver calls for page.

Workaround is to create special cache, where you storage widths for already
"tested" chars for given font. Actually it is not trivial task,sometimes very
non-trivial, but even simplest implementation I tested here gives us fantastic
speed improvement.

Current price is that for each mFontMetrics object ( it uses additional memory 
-  float foo[OxFFFF]. Unfortunately.
With time and work it may be reduced and optimized, but for me it is already
worth submitting and testing. Because even with simpicistic implementation
performance boost is fantastic.

Comment 1

[Sergei Dolgov]

Attachment: patch - draft implementation

Preliminary but worth testing.

Comment 2

[Sergei Dolgov]

Comment on attachment 198155 [details] [diff] [review]
patch - draft implementation

something went wrong in if()s

Comment 3

[Sergei Dolgov]

Attachment: patch

This one seems working again

Comment 4

[Sergei Dolgov]

Notice for curious.
Previous patch is rather proof of concept. Being fastest but memory hog. Kind of.
Actual implementation will use hash-table.
In future there is sence to make it global, but it needs rewrite of
nsFontMetricsBeOS in order to make cacheable (and hasheable) font descriptions,
and to resolve possible thread-safety problem.
Thus, at the moment it looks like good idea to restrict ourselves only by
font-width cache unique for nsFontMetrics instance.

Comment 5

[Sergei Dolgov]

Attachment: patch with hashtable

Reimplemented using nsHashtable.
For key generating from single UTF-8 character I use nsCStringKey().

Biesi, timeless, can you look at?
I never used nsHashtable, do don't know, for example, if simple Reset() is
enough 
 to free all new-ed width entries (as hashtable gets those as (void *)).

Comment 6

[Sergei Dolgov]

Attachment: patch

taking in account some timeless notices,
logic is same asin previous

Comment 7

[tqh]

This patch isn't working, it has different variablename for hashtable in header
and source.

Comment 8

[Sergei Dolgov]

Attachment: patch - for whole strings

Here is patch which tries to calculate whole string width from cached char
widths.
But it works sometimes bit strange - some strings get too wide-spaced.
Performance of this version is higher than previous, but issue needs
investigation.
At moment I prefer to have previous patch as production-ready.

Comment 9

[tqh]

The last two patches are broken. You use mFontWidthCache in code and
mWidthFontCache in header. I wonder if you tested the right builds. 

Comment 10

[Sergei Dolgov]

Attachment: patch with nsDataHashtable

Using nsDataHashtable.
Single-char version.
Also added explicit setting for B_FIXED_SPACING for font - char and string
spacing don't work for us here anyway.
Removed direct links to system fonts in fallback case - that was bit dangerous
and didn't allow to control some font parameters.
Changed line in EnumFonts according biesi comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=310680#c10

Comment 11

[Sergei Dolgov]

Comment on attachment 198222 [details] [diff] [review]
patch with nsDataHashtable

review request

Comment 12

[tqh]

Comment on attachment 198222 [details] [diff] [review]
patch with nsDataHashtable

r=[e-mail]

I'm not very good at the utf-'magic' but it looks good to me and seems to work
ok.

Comment 13

[Sergei Dolgov]

Patch landed in trunk:
nsFontMetricsBeOS.cpp new revision: 1.43; previous revision: 1.42
nsFontMetricsBeOS.h new revision: 1.13; previous revision: 1.12
nsRenderingContextBeOS.cpp new revision: 1.55; previous revision: 1.54

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

might the APIs from bug 297074 help here too?

Comment 15

[Sergei Dolgov]

There must be little follow-up to this bug.
Curently "biggest" versopn nsRenderingContextBeOS::::GetTextDimensions() uses
mFont->StringWidth() - it means non-cached BView::StringWidth();

For most time, in theory, GetTextDimensions asks for longer than one character
strings, so it shouldn't matter with current GetStringWidth() implementation,
but in future it might do. So  I will move it to unified way.
Second problem is BeOS and Mozilla different understanding about widths.
In other cases it differs by one, but i'm yet sure if it matters for
StringWidth(). Looks like it does, so why i failed to calculate, even with fixed
spacing, string width from cached char widths.

Comment 16

[Sergei Dolgov]

Thanks biesi.
As i already planned changes in nsRenderingContext, i will look at bug you
pointed. Though, patch looks huge there:)

Comment 17

[Niels Reedijk]

Comment on attachment 198222 [details] [diff] [review]
patch with nsDataHashtable

Requesting approval for landing in the MOZILLA_1_8_BRANCH.

This is a BeOS-only change and does not affect other platforms.

Comment 18

[Mike Schroepfer]

Comment on attachment 198222 [details] [diff] [review]
patch with nsDataHashtable

a=schrep for drivers.

Comment 19

[Sergei Dolgov]

Checking in mozilla/gfx/src/beos/nsFontMetricsBeOS.cpp;
/cvsroot/mozilla/gfx/src/beos/nsFontMetricsBeOS.cpp,v  <--  nsFontMetricsBeOS.cpp
new revision: 1.40.12.3; previous revision: 1.40.12.2
done
Checking in mozilla/gfx/src/beos/nsFontMetricsBeOS.h;
/cvsroot/mozilla/gfx/src/beos/nsFontMetricsBeOS.h,v  <--  nsFontMetricsBeOS.h
new revision: 1.12.12.1; previous revision: 1.12
done
Checking in mozilla/gfx/src/beos/nsRenderingContextBeOS.cpp;
/cvsroot/mozilla/gfx/src/beos/nsRenderingContextBeOS.cpp,v  <--  nsRenderingContextBeOS.cpp
new revision: 1.51.12.4; previous revision: 1.51.12.3
done