652724 - Use cached pref value for gfx.font_rendering.harfbuzz.scripts

Status: RESOLVED INVALID

(Core :: Layout: Text and Fonts, defect)

Description

[Daniel Veditz [:dveditz]]

Was browsing through some of the changes for Fx5 and noticed what looked like a lot of pref reading in the patch for bug 623495

Maybe I'm reading the patch badly, but it looks like every InitTextRun() you call UseHarfBuzzForScript() which in turn gets the pref service and reads a pref. This seems wasteful for something that will hardly ever change, and certainly won't while you're laying out a single page.

You might get a measurable perf boost on text-heavy pages if you switched to a cached pref value. Look into the helper function nsContentUtils::AddIntPrefVarCache() which makes this easy -- just use the static variable you pass in when you need the value instead of a function call. Looks like you maybe tried to cache the value since I see that pref in your pref-changed callback (where you invalidate the font cache?), but I think you're still calling the pref service every InitTextRun().

There's a similar helper func for boolean prefs which could replace all your uses of gfxFont::GetBoolPref(), similar perf pain of getService overhead on each use.

Comment 1

[Jonathan Kew [:jfkthame]]

(In reply to comment #0)
>  it looks like every InitTextRun() you
> call UseHarfBuzzForScript() which in turn gets the pref service and reads a
> pref

I don't think this is correct, as I read the code. It only gets the pref service and reads a pref if the gfxPlatform's mUseHarfBuzzScripts is currently UNINITIALIZED_VALUE - but then it saves the value here, and following calls don't touch the pref service at all unless it gets reset (by the pref-changed observer) in the meantime.

IOW, the current setting of the pref is cached by the gfxPlatform object; it's initialized from the pref service the first time it's needed, but then only re-read if it has changed.

(The code would look a little simpler if we initialized the variable during gfxPlatform::Init(), rather than only on first use, and recorded changes immediately in the observer. But we seem to have a pattern here of not reading prefs until they're actually needed for the first time, probably in an effort to minimize work done on the actual startup path.)

> Look into the helper function
> nsContentUtils::AddIntPrefVarCache() which makes this easy -- just use the
> static variable you pass in when you need the value instead of a function call.

We could use that helper to watch for changes, but that would mean we'd have an additional observer for this pref - as we'd still need to observe it in order to clear the font/textrun caches. Splitting the functionality over two observers instead of one doesn't seem like a win.

Comment 2

[Daniel Veditz [:dveditz]]

Sorry about that -- you are quite right.