Beginning on October 25th, 2016, Persona will no longer be an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 652754 - pages containing korean cause font tables to be read at startup
: pages containing korean cause font tables to be read at startup
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: ---
Assigned To: John Daggett (:jtd)
: Milan Sreckovic [:milan]
Depends on: 651498
  Show dependency treegraph
Reported: 2011-04-26 00:11 PDT by John Daggett (:jtd)
Modified: 2011-05-20 02:45 PDT (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch, eliminate unnecessary font data enumerations at startup (11.35 KB, patch)
2011-04-26 20:13 PDT, John Daggett (:jtd)
no flags Details | Diff | Splinter Review
patch v2, eliminate unnecessary font data enumerations at startup (14.03 KB, patch)
2011-05-09 21:29 PDT, John Daggett (:jtd)
jfkthame: review+
Details | Diff | Splinter Review

Description John Daggett (:jtd) 2011-04-26 00:11:05 PDT
Code was added for FF4 to try and avoid having to enumerate localized font names if at all possible.  Unfortunately, there's still a scenario under which those fonts will be enumerated: when using GDI font rendering, if a page contains Korean with no font specified, we look up pref fonts for Korean.  Our pref font setting for Korean uses the localized name, which in turns kicks off the enumeration process.  The fix is to use the en-us name and make sure our UI code is provided with the localized name.

Steps to reproduce:

1. Start FF4 and enter about:config
2. Set 'gfx.directwrite.disableD2D' to true to assure GDI
3. Set as the default home page
4. Quit Firefox
5. Run ProcessMonitor
6. Start Firefox
7. After the page loads, switch to ProcessMonitor

Result: ProcessMonitor shows that all fonts on the system have been enumerated (at cold startup this will include a read but on warm startup only a stat call will occur).

Note: this is currently blocked by 651498, gfx code is not currently handling pref settings correctly at startup (Nightly builds on 4/20 onwards).
Comment 1 John Daggett (:jtd) 2011-04-26 20:13:41 PDT
Created attachment 528518 [details] [diff] [review]
patch, eliminate unnecessary font data enumerations at startup

Part of this patch is cleanup, trimming out unused member variables.  mUnicodeFont and mSymbolFont were initialized in the GDI font entry case by reading the cmap.  I trimmed mUnicodeFont and made IsSymbolFont() call to check to make sure the cmap is loaded when needed.  This eliminates unneeded cmap loads when enumerating the faces in a given family.

The other part was to switch over to using the en-us names of pref fonts.  This eliminates the "miss on localized name, load localized names, lookup on localized name" that was causing name table reads to occur at startup.  The testcase was hitting this path by containing "Language" UI strings in Korean.  This would then fallback to the pref fonts for Korean resulting in a localized name lookup miss.

Without the patch, the GDI path loads 19MB of font data at cold startup for the testcase, 2.3MB with the patch.

Testing the fix requires a fix for the gfx prefs init problem (bug 651498) but that should be fixed in the next couple days.
Comment 2 John Daggett (:jtd) 2011-04-26 20:21:17 PDT
We will mourn the passing of the 'IsCrappyFont' method...
Comment 3 John Daggett (:jtd) 2011-05-09 21:29:41 PDT
Created attachment 531242 [details] [diff] [review]
patch v2, eliminate unnecessary font data enumerations at startup

Note that some of the names in all.js were spelled incorrectly (i.e. didn't match the actual font names!).  A couple others were mislabeled in the comments.  I went through and verified with a testcase that the English names match the identical font matched using the localized name.  Also trimmed out a bunch of trailing white space.
Comment 4 Jonathan Kew (:jfkthame) 2011-05-11 08:57:40 PDT
Comment on attachment 531242 [details] [diff] [review]
patch v2, eliminate unnecessary font data enumerations at startup

Review of attachment 531242 [details] [diff] [review]:

Looks fine to me.

::: modules/libpref/src/init/all.js
@@ +1434,5 @@
>  pref("", "Guttman Yad, Ktav, Arial");
> +pref("", "MS PMincho"); // "MS PMincho"
> +pref("", "MS PGothic"); // "MS PGothic"
> +pref("", "MS Gothic"); // "MS Gothic"

These comments (and similar ones later) are now completely redundant; I'd suggest stripping them.
Comment 5 John Daggett (:jtd) 2011-05-19 23:10:53 PDT
Pushed to trunk
Comment 6 Jonathan Kew (:jfkthame) 2011-05-20 02:27:56 PDT
Re-opening; backed out (by jtd) because of Linux64 build redness:

Log of the failed job:

obj-firefox/dist/bin/leakstats starting at Thu May 19 16:31:29 2011
Unknown event type 0x1
obj-firefox/dist/bin/leakstats: log file incomplete
program finished with exit code 1

That looks to me like an intermittent build or infra failure rather than a code issue. So I re-triggered that job, and it's green on the second attempt:
Comment 7 Jonathan Kew (:jfkthame) 2011-05-20 02:45:11 PDT
Re-landed the original patch, as I don't believe it was at fault:

Note You need to log in before you can comment on or make changes to this bug.