375760 - Rework gfxTextRun cache

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We need a new textrun cache that is uses nsExpirationTracker for low-overhead expiry and works with the new textframe (which keeps state in the textruns, because for example line breaks can require changes to glyph shaping). My plan is to have each gfxFont object hold a hashtable referencing the textruns that were entirely matched by the font, see
http://groups.google.com/group/mozilla.dev.tech.gfx/browse_thread/thread/60ddf7dcf27388fd/817b529e1fbf6fca

A global nsExpirationTracker will manage the lifetimes of textruns. As textruns expire we actually rip them out of the frames that they're involved in (and remove cached references) because the frames can recreate their textruns as necessary. This will be managed from content/layout.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: patch

Here's the new textrun cache. Some details:
-- Attaches the text for each textrun to the textrun. This is needed to verify cache hits. This means we have to thread the text through various API calls.
-- Store the textrun's fontgroup in the textrun. Needed so we can verify cache hits correctly.
-- Now that textruns never have to re-get their text, we can remove RememberText and associated APIs. The aProvider parameter to SetLineBreaks can also be removed (it was only needed in case text needed to be recovered from layout).
-- Since we're changing MakeTextRun's signature, I also made the Parameters* parameter const, and broke out the flags into a separate parameter because various sites want to change the flags on the way through.
-- Factors the textrun cache into two pieces: a generic piece that doesn't manage textrun lifetimes, just provides hash lookup to find existing textruns; and a "global textrun cache" that adds auto-expiry of textruns after 30 seconds and returns a transient reference to any callers. This is suitable for use by nsThebesRenderingContext/FontMetrics and SVG. The new textframe will define its own textrun cache which shares the generic piece but applies its own lifetime management policy.
-- Cache space glyph ID in gfxFont implementations and provide MakeSpaceTextRun/MakeEmptyTextRun APIs on gfxFontGroup that will be used by new textframe.
-- Added gfxTextRun::Clone API for use by the cache when the cache user demands a new textrun be created when there's a cache hit.
-- Fix some documentation/naming bugs in gfxTextRun where simple glyph advances are actually in appunits, not pixels.
-- Remove fontgroup special-string APIs; we can use the textrun cache instead and do it all in the textframe code.
-- Ensure that passing a null aProvider always means "no spacing" even if spacing is enabled for the textrun.
-- Fix bug in gfxPangoFont::Measure that was treating advances as being in pixels intead of appunits (only called by new textframe)
-- Create nsTextFrameTextRunCache.h with methods to initialized and shutdown a text-frame specific textrun cache. nsTextFrame.cpp doesn't use such a cache, so it stubs out those methods. nsTextFrameThebes.cpp will provide a real implementation of the same methods.
-- Make SVG use the global textrun cache. The textruns are then owned by the global textrun cache and auto-expired on a timer, so SVG doesn't need to clean them up anymore.

Sorry the patch is so big. A few small bits could be split out.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: new-textframe update

This is the corresponding patch to the new-textframe and related NPOB code to a) get them to work with the API changes and b) get new-textframe to use a textrun cache and the new MakeEmptyTextRun and MakeSpaceTextRun APIs.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 263116 [details] [diff] [review]
patch

I've fixed a subtle bug and I think I should break the patch up a bit. I'll post new patches soon.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: part 1: update textrun/font APIs

OK, here's the first part of the patch. This is solely about cleaning up/adding some APIs to gfxTextRun/gfxFont/gfxFontGroup.

-- Make the text always accessible to the textrun, and optionally owned by it (when TEXT_IS_PERSISTENT is not set). This obviates the need for text reconstruction APIs, which are removed. It also means SetLineBreaks needs to PropertyProvider parameter.
-- Make the Parameters* parameter to MakeTextRun const, and pass the flags separately.
-- remove mLangGroup parameter from Parameters (it's redundant with the gfxFontGroup)
-- Add gfxFont::GetSpaceGlyph API
-- Store fontgroup in the textrun
-- Make userdata settable in the textrun
-- Add gfxTextRun::Clone API
-- Add gfxTextRun::GetExpirationState, which the new textrun cache will need
-- Add gfxFontGroup::MakeEmptyTextRun/MakeSpaceTextRun APIs
-- Remove SpecialString APIs (new textframe can do this itself just as effectively and it will perform fine thanks to the textrun cache)
-- Make gfxTextRun destructor virtual because it can have subclasses

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: part 1 v2

The previous version incorrectly set TEXT_IS_PERSISTENT on the corner case when there is a hit in the cache but the hit textrun is not usable so we create a new textrun. This fixes that bug.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

the patch conflicts to the patch for bug 357637 :-(

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: part 1, v3

Sorry, the previous patch forgot to clone the string in the !TEXT_IS_PERSISTENT case when the textrun takes ownership of the string. This fixes that.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #6)
> the patch conflicts to the patch for bug 357637 :-(

I think the conflict will be easy to fix

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 263583 [details] [diff] [review]
part 1, v3

Looks good, nothing jumps out at me -- I assume reftests and all that are happy; if so, r=me

Comment 10

[Robert Sayre]

This is making windows burn.

Comment 11

[Robert Sayre]

I backed this out due to bustage. sorry.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry me. I was paying a bit of attention to tinderbox but not enough. Also, it busted Linux Tp somehow. Blarg.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Landed successfully this time.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: new cache

The actual new cache.

There are really two caches. There's a base gfxTextRunCache which handles lookup but doesn't manage textrun lifetime. Then there's a gfxGlobalTextRunCache which works like the current cache, managing textrun lifetimes. Its lifetime management uses an nsExpirationTracker.

As we discussed earlier, textruns are cached based on the font, if all characters were resolved by the first font in a fontgroup, otherwise they're cached based on the fontgroup.

My new-textframe update will use a new textrun cache based off gfxTextRunCache for its needs. nsIRenderingContext-based text rendering will continue using the gfxGlobalTextRunCache. I also have a patch to make SVG use the gfxGlobalTextRunCache (currently it does not use a cache).

Comment 15

[Simon Montagu :smontagu]

Um, is there going to be an update of nsTextFrameThebes.cpp in the near future to go with this checkin?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, as soon as the new cache lands.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 264203 [details] [diff] [review]
new cache

Looks fine -- but can you mark the previous attachments as obsolete as appropriate?

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked that in.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This reduced Tp significantly (3-10%) on all platforms. That's probably a combination of eliminating lots of calls to PR_IntervalNow, and better caching now that textruns are often cached on gfxFonts, which are shared globally.