87224 - [IBENCH] inefficiencies in nsFontGTKSubstitute

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Chris Waterson]

Did a jprof profile of a stock iBench run, and discovered that about 5% of the
callstacks ended up nsFontGTKSubstitute::Convert(), which spends most of its
time in nsSaveAsCharset::Convert(). The big problem here is that most of the
time is spent loading and parsing font properties files. The call stack looks like:

  nsSaveAsCharset::Convert
  nsEntityConverter::ConvertToEntities
  nsEntityConverter::GetVersionPropertyInst
  nsEntityConverter::LoadEntityPropertyFile

We should look into this. Can these property files be cached? Or can the values
themselves be cached? Does text measurement really need to be doing entity
substitution? Anyway, if we can nail this, looks like it's a quick 5% speedup on
iBench for Linux. (It's probably related to the Windows-1252 character set
that's used on these pages.)

Comment 1

[Chris Waterson]

cathleen: here's another iBench bug (this one is Unix only). Do you have a
tracking bug for these?

Comment 2

[Darin Fisher]

i wonder if the significance of this isn't somehow related to the double reload / 
charset change.

Comment 3

[rbs]

Another problem that may be worsening this is that FindSubstituteFont (on 
GfxGTK) is stateless - Bug 78456. Hence, although a character may not have a 
glyph, it is looked up again and again, causing the font properties files
to be loaded and parsed each time.

There are several consumers of the property files (e.g., the parser when mapping
the NCRs), so if they have to be cached, it might be more suitable at a higher, 
sharable level, than GFX.

Another fix might be to let FindSubstituteFont() cache the missing characters as 
they become known, as suggested in Bug 78456.

Comment 4

[nhottanscp]

Reassign to ftang.

Comment 5

[Frank Tang]

we do cach the nsISaveAsCharset in nsFontGTKSubstitute in the 
nsFontMetricsGTK.cpp. See the following:

2095 int nsFontGTKSubstitute::gCount = 0;
2096 nsISaveAsCharset* nsFontGTKSubstitute::gConverter = nsnull;
2097 
2098 nsFontGTKSubstitute::nsFontGTKSubstitute(nsFontGTK* aFont)
2099 {
2100   gCount++;
2101   mSubstituteFont = aFont;
2102 }
2103 
2104 nsFontGTKSubstitute::~nsFontGTKSubstitute()
2105 {
2106   if (!--gCount) {
2107     NS_IF_RELEASE(gConverter);
2108   }
2109   // Do not free mSubstituteFont here. It is owned by somebody else.
2110 }

The problem is we relase it when the gCount go to zero. Basicailly, if you only 
have one window open or only one window use the nsFontGTKSubstutite, the count 
will go zero when we load the new page. 

It seems eas to fix since we already cache it and have another global caching 
mecahism in place. I try my patch and it release it only when we quit (or all 
font got released) 

Comment 6

[Frank Tang]

Attachment: patch

Comment 7

[Frank Tang]

ok. here is a summary of the patch:
1. we move nsFontGTKSubstitute::gConverter from class global (static inside 
class) to file global (static inside file) and rename it gFontSubConverter

the reason to move it up is we need to release it in FreeGlobals function 
instead of in the nsFontGTKSubstitute destructor
I rename gConverter to gFontSubConverter

2. I add NS_IF_RELEASE(gFontSubConverter); in FreeGlobals()
3. I remove the gCount inside nsFontGTKSubstitute  since it is origional design 
to cache the gConvert inside the class but we move it up now. 
4. Change all gConverter to gFontSubConverter (simple name change)


I will ask bstell to code review today. rbs, can you also code review?
waterson, can you try my patch . It will be nice if you can spend some time 
teach me and bstell about how to use jprof. 

Comment 8

[Chris Waterson]

Changes look good to me. [s]r=waterson, if you need it. Might we need to make 
similar changes on the other platforms? Also, could you throw some before/after 
iBench numbers into the bug?

Comment 9

[Frank Tang]

waterson- 
>could you throw some before/after iBench numbers into the bug?
do I need to build a release build to do that. It take me about 4 hours to 
compile the whole tree. My current linux box is very very very slow. I will ask 
bstell to help. Or you think debug build number is OK ?

Comment 10

[Chris Waterson]

I've got an opt build. Here's what I see, over DSL, to zdnet's site (so beware
noise!)

before: 186.69/59.85/18.12
after:  181.41/60.65/17.25

I think the number to pay attention to here is the cached load time (my
configuration is going to suffer from network noise, so the slight increase in
first-page time is probably bogus -- I got my other set of numbers at 6am this
morning). Based on that, this looks good! (About a 5% improvement.)

Anyway, just to be sure, get bstell to do an opt build and run jrgm's tests as
well as ibench on dogspit.mcom.com.

Comment 11

[Frank Tang]

my machine is simply too slow . The number is like 2364.61/398.04/280.94 after 
(Not trying before yet). you can see how slow my machine is....

Comment 12

[Frank Tang]

ok. we got optimize build on bstell's machine now. running the ibench w/o my 
patch now.

Comment 13

[Frank Tang]

OK, I got the number from bstell's opt build:
before the patch: 385.59/55.72/47.12
after  the patch: 365.1/53.23/44.57
The % of improvement is
5.28% / 4.47% / 5.41%

Comment 14

[Frank Tang]

check into trunk. need to check into 92 branch

Comment 15

[kill this account]

r=[e-mail]

Comment 16

[tor]

a=tor for 0.9.2 branch checkin

Comment 17

[Christopher Blizzard (:blizzard)]

tracking, tracking

Comment 18

[Frank Tang]

fix and check in to both trunk and branch.

Comment 19

[Andreas Becker]

Switching qa contact to ftang for now and CC ylong.  Frank, can this be verified
within development or can you provide IQA with a test case?  Thanks.