1375454 - Avoid double hashtable lookup when instantiating a new gfxFont and adding to the cache

Status: RESOLVED WONTFIX

(Core :: Graphics: Text, enhancement)

Description

[Jonathan Kew [:jfkthame]]

In gfxFontEntry::FindOrMakeFont, we first try to look up a gfxFont in the global gfxFontCache; then if it is not found, we instantiate a new gfxFont and add it to the cache, which involves repeating the hashtable lookup.

We can save a few milliseconds by using a hashtable API that combines the lookup of an existing entry and insertion of a new one if needed.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Avoid double hashtable lookup when adding a new gfxFont instance to the global cache

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Avoid double hashtable lookup when adding a new gfxFont instance to the global cache

Comment 3

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa59a31c18b45d7185e1445f7d3af8f112300b70

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8880371 [details] [diff] [review]
Avoid double hashtable lookup when adding a new gfxFont instance to the global cache

Review of attachment 8880371 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.cpp
@@ +285,5 @@
>  void
>  gfxFontCache::FlushShapedWordCaches()
>  {
>      for (auto it = mFonts.Iter(); !it.Done(); it.Next()) {
> +        if (it.Get()->mFont) {

It seems like we shouldn't need to check null because we shouldn't be putting null's into the hash table.

Comment 5

[Jonathan Kew [:jfkthame]]

I re-checked and confirmed that removing these null-checks results in crashes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=50ecea7271b6628bf51e9e8d70e4529007a6ec10.

After entirely too much time reading the code, I think I figured out what's breaking: on Linux, there is a possibility that FindOrMakeFont is called recursively via the gfxFcPlatformFontList implementation of CreateFontInstance (which calls ChooseFontSize -> SizeForStyle -> GetAspect -> FindOrMakeFont).

This means that the usage of hashEntry in

    gfxFontCache::HashEntry* hashEntry;
    if (gfxFontCache::GetCache()->EnsureInserted(this, aStyle, aUnicodeRangeMap,
                                                 &hashEntry)) {
        gfxFont* newFont = CreateFontInstance(aStyle, aNeedsBold);
        if (newFont && newFont->Valid()) {
            newFont->SetUnicodeRangeMap(aUnicodeRangeMap);
            hashEntry->mFont = newFont;
        }

is unsafe: the CreateFontInstance call may lead to a recursive call back to FindOrMakeFont, which can cause hashtable mutation and potentially reallocation, so by the time we try to set its mFont field, it has moved and the pointer is invalid.

So because of the recursion here, we can't use the approach of this patch at all (or any equivalent that depends on looking up the entry before instantiating the new font, and then expecting that reference to remain valid); we have to do a new lookup after instantiating the font.

--> WONTFIX