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

RESOLVED WONTFIX

Status

()

Core
Graphics: Text
RESOLVED WONTFIX
6 months ago
5 months ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 months ago
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.
(Assignee)

Comment 1

6 months ago
Created attachment 8880357 [details] [diff] [review]
Avoid double hashtable lookup when adding a new gfxFont instance to the global cache
Attachment #8880357 - Flags: review?(jmuizelaar)
(Assignee)

Updated

6 months ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Comment 2

6 months ago
Created attachment 8880371 [details] [diff] [review]
Avoid double hashtable lookup when adding a new gfxFont instance to the global cache
Attachment #8880371 - Flags: review?(jmuizelaar)
(Assignee)

Updated

6 months ago
Attachment #8880357 - Attachment is obsolete: true
Attachment #8880357 - Flags: review?(jmuizelaar)
(Assignee)

Comment 3

6 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa59a31c18b45d7185e1445f7d3af8f112300b70
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.
Attachment #8880371 - Flags: review?(jmuizelaar) → review-
(Assignee)

Comment 5

5 months ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.