418536 - cleanup string handling in font code

Status: REOPENED

(Core :: Graphics: Text, defect, P4)

Description

[Benjamin Smedberg]

From bug 416411 comment 17:

dmandelin found a reduced test case where about 20% of ~110,000 nsAutoStrings are assigned to an nsString: loading 

    http://zh-yue.wikipedia.org/wiki/%E7%BE%8E%E5%9C%8B

The highest frequency contexts for the assign are below, in this format:

[freq] [sourceline]
      [source code]
      [additional info on stack trace]

  freq = % of nsAutoString assigns accounted for in this item
  sourceline = source line of deepest non-libxpcom call on stack at assign
  source code = code of that line

44% modules/libpref/src/nsPrefBranch.cpp:343 (in nsPrefBranch::GetComplexValue)
      rv = theString->SetData(NS_ConvertUTF8toUTF16(utf8String));
      [Mostly called from AppendPrefFonts via CopyUnicharPref.]

15% gfx/thebes/src/gfxFont.cpp:671 (in ResolveData::ResolveData)
      mGenericFamily(aGenericFamily),
      [Mostly called from AppendPrefFonts via gfxFontGroup::ForEachFont.]

15% gfx/thebes/src/gfxQuartzFontCache.mm:205 
      (in gfxQuartzFontCache::GenerateFontListKey)
      aResult = aKeyName;
      [Mostly called from AppendPrefFonts via gfxFontGroup::ResolveFontName.]

15% gfx/thebes/src/gfxPlatform.cpp:253 (in AppendGenericFontFromPref)
      genericDotLang.Assign(NS_ConvertUTF16toUTF8(genericName));
      [Mostly called from AppendPrefFonts.]

Overall, 88% of the nsAutoStrings that got assigned to another nsString had a
call to AppendPrefFonts when Assign was called.

dmandelin, was this on mac?

Comment 1

[David Mandelin [:dmandelin]]

Yes, it was on Mac.

Comment 2

[John Daggett (:jtd)]

(In reply to comment #0)
> From bug 416411 comment 17:
> 
> dmandelin found a reduced test case where about 20% of ~110,000 nsAutoStrings
> are assigned to an nsString: loading 
> 
>     http://zh-yue.wikipedia.org/wiki/%E7%BE%8E%E5%9C%8B
> 

I think a lot of this was fixed by bug 409432, which went in on 1/30.  We were building pref font lists per *character*, leading to a lot of string thrashing.  This string thrashing should now only occur once after startup, pref font lists are then cached.

Comment 3

[Boris Zbarsky [:bzbarsky]]

My analysis of the stuff is in bug 416411 comment 19.  If we do in fact cache this stuff now, that's great news.  Some of the string code is still pretty iffy, but if it's not called much it's less of an issue.

Comment 4

[David Mandelin [:dmandelin]]

I reran the measurements with cvs trunk as of 19 Feb 2008. Now, 3.6% of autostrings are assigned to other strings, which is in line with the numbers for other pages I've tested. I'll leave the decision on how to mark the bug to those who actually the code.

Comment 5

[John Daggett (:jtd)]

Not a blocker but it's probably a good idea to clean up the string code noted in bug 418536 comment 19:

> 15% gfx/thebes/src/gfxFont.cpp:671 (in ResolveData::ResolveData)

I just looked at the consumers, and it looks to me like mGenericFamily could
just be a |const nsACString&|.  Then we'd avoid the string copy altogether. 
Probably worth a separate bug as well.

> 15% gfx/thebes/src/gfxQuartzFontCache.mm:205 

This one is tough, since we copy and immediately modify.  This might or might
not work better with the two-arg form of ToLowerCase.  It also might or might
not work better if callers can sort out whether they already have a lowercased
string...

> 15% gfx/thebes/src/gfxPlatform.cpp:253 (in AppendGenericFontFromPref)

The code here should be more like:

  NS_ConvertUTF16toUTF8 genericDotLang(genericName);

That would save an object construction and a string copy.

There's similar code above:

  279         genericName = NS_ConvertASCIItoUTF16(aGenericName);

That should likely be:

  CopyASCIItoUTF16(aGenericName, genericName);

Comment 6

[Ryan VanderMeulen [:RyanVM]]

John, is this still wanted?

Comment 7

[John Daggett (:jtd)]

Yes, this code still hasn't been cleaned up and should be.