implement async font loader

RESOLVED FIXED in mozilla29

Status

()

defect
P1
normal
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: jtd, Assigned: jtd)

Tracking

(Blocks 2 bugs)

Trunk
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 3 obsolete attachments)

Assignee

Description

6 years ago
[moved from bug 752394 so that this can land separately]

I'm breaking out the async font loader implementation into a separate
bug so that I can land it first before landing other patches that fix
related font chrome hang bugs.  The implementation here simply
duplicates the existing functionality of the current font loader code
which runs on the main thread in intervals until it completes.

Current font loader sequence:

* platform font list initialization (main thread)
  ==> StartLoader - starts a timer to delay font loading until after startup
      ==> InitLoader - initialize list of gfxFontFamily objects

* timer fires after delay (main thread)

* timer fires on an interval (main thread)
  ==> RunLoader - iterates over a subset of gfxFontFamily objects,
                  initializing whatever is needed on a given platform
                  (cmaps, facenames, localized family names) 

* RunLoader returns true (main thread)
  ==> FinishLoader - clean up


Async font loader sequence:

* platform font list initialization (main thread)
  ==> StartLoader - starts a timer to delay font loading until after startup
      ==> InitLoader - initialize list of font family name strings

* timer fires after delay (main thread)
  ==> StartLoader - creates a platform-specific FontInfoData object, then
                    creates a new thread and dispatches an AsyncFontInfoLoader
                    runnable to the new thread

* AsyncFontInfoLoader::Run (off-main-thread)
  ==> FontInfoData::Load - iterates over font families
      ==> LoadFontFamilyData - platform-specific routine to which initializes
                               whatever is needed on a given platform
                               (cmaps, facenames, localized family names) 
  dispatches FontInfoLoadCompleteEvent runnable to main thread

* FontInfoLoadCompleteEvent::Run (main thread)
  ==> FinalizeLoader - iterates over gfxFontFamily objects,
                       transfering data loaded on async thread.
                       calls LoadFontInfo and starts interval timer if can't
                       complete within set timeframe

* timer fires on an interval (main thread)
  ==> LoadFontInfo - additional calls to LoadFontInfo

* LoadFontInfo returns true (main thread)
  ==> CleanupLoader - clean up

After the loader completes, the FontInfoData object is dumped, leaving
only the data that was explicitly transfered to the platform font list
and gfxFontFamily objects.

Loader infrastructure base class and async runnables are in
gfxFontInfoLoader.[h/cpp].  gfxPlatformFontList::LoadFontInfo contains
the code that transfers the loaded font data to the platform font
list. Platform-specific loading code is contained in
LoadFontFamilyData methods for each platform font list (OSX,
DirectWrite, GDI).  The gfxFontFamily methods that initialize cmaps or
name data are now passed a FontInfoData object containing font data
loaded on an async thread.

The LoadFontFamilyData methods are run on the async thread and so must
use only static methods or thread-safe platform API's, they explicitly
don't touch any object accessed by text code running on the main
thread, nor do they access any harfbuzz method.

The interval timer to transfer font data back to the platform font
list generally isn't needed; since the transfer is basically just
swizzling around data structures, the transfer will usually occur
within the initial allotted time slice. For systems with huge numbers
of fonts (i.e. >3000 fonts) or in cases where simply enumerating fonts
takes a long time (due to overzealous virus-checker code for example)
it may take longer and the interval timer will fire multiple times.

Note: loader code doesn't run on Android or Linux.
Assignee

Comment 1

6 years ago
Switch to using CTFontManager instead of AppKit since it's not clear AppKit methods are thread-safe (Apple docs don't indicate either way).
Assignee

Comment 2

6 years ago
Move existing methods used in reading font tables out to static methods which can be used off-main-thread.
Assignee

Comment 3

6 years ago
Move the gfxFontInfoLoader class out from gfxFontUtils.[h/cpp] into a separate file before making changes to it.
Assignee

Comment 4

6 years ago
This is the bulk of the implementation of the async font loader:
  - loader now uses async runnable
  - platform-specific code for loading font data under OSX
  - code for transferring data to platform font list after async loader completes
Assignee

Comment 5

6 years ago
Font data loading for DirectWrite platform font list.
Assignee

Comment 6

6 years ago
Font data loading for GDI platform font list.
Assignee

Comment 7

6 years ago
EnumFontFamiliesExW calls callback one per style-charset rather than simply per style, so skip subsequent calls with the same style.
Attachment #8363484 - Attachment is obsolete: true
Attachment #8364210 - Flags: review?(bas)
Assignee

Updated

6 years ago
Attachment #8363477 - Flags: review?(roc)
Assignee

Updated

6 years ago
Attachment #8363478 - Flags: review?(roc)
Assignee

Updated

6 years ago
Attachment #8363481 - Flags: review?(roc)
Assignee

Updated

6 years ago
Attachment #8363482 - Flags: review?(roc)
Assignee

Updated

6 years ago
Attachment #8363483 - Flags: review?(bas)
Assignee

Comment 8

6 years ago
Parts 1,2,3 are refactorings and moving code into a new file. Part 4 is the key piece that needs careful review since it's multi-threaded. Parts 5 & 6 are primarily reviews of Windows DirectWrite/GDI usage.
Assignee

Updated

6 years ago
Attachment #8363477 - Flags: review?(roc) → review?(bas)
Assignee

Updated

6 years ago
Attachment #8363478 - Flags: review?(roc) → review?(bas)
Assignee

Updated

6 years ago
Attachment #8363481 - Flags: review?(roc) → review?(bas)
Assignee

Updated

6 years ago
Attachment #8363482 - Flags: review?(roc) → review?(bas)
Assignee

Comment 9

6 years ago
Bas, roc is on holiday and jfkthame is off for a bit, would you be able to tackle reviewing all these?
Comment on attachment 8363477 [details] [diff] [review]
part 1, use CTFontManager API to enumerate font families in OSX

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

::: gfx/thebes/gfxMacPlatformFontList.mm
@@ +681,2 @@
>  
> +    CFArrayRef familyNames = CTFontManagerCopyAvailableFontFamilyNames();

https://developer.apple.com/library/mac/documentation/Carbon/Reference/CoreText_FontManager_Ref/Reference/reference.html#//apple_ref/c/func/CTFontManagerCopyAvailableFontFamilyNames

states: This function returns a retained reference to a CFArray of CFStringRef objects representing the visible font family names of the available fonts, or NULL on error. The caller is responsible for releasing the array.

I don't see this being released :). I believe you'll need a simple CFRelease from what I can tell.
Attachment #8363477 - Flags: review?(bas) → review-
Comment on attachment 8363478 [details] [diff] [review]
part 2 - create static helper methods for fontinfo loading

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

::: gfx/thebes/gfxFont.cpp
@@ +1157,2 @@
>      const gfxFontUtils::NameHeader *nameHeader =
> +        reinterpret_cast<const gfxFontUtils::NameHeader*>(aNameData);

Personally I'd prefer to see this happen with a typesafe helper rather than a reinterpret cast :).

@@ +1208,5 @@
> +    ReadOtherFamilyNamesForFace(mName, nameData, dataLength,
> +                                otherFamilyNames, useFullName);
> +
> +    uint32_t i, n = otherFamilyNames.Length();
> +    for (i = 0; i < n; i++) {

nit: Can we just declare in the for like we do everywhere else? This is just me but I was a little bit confused by the syntax.

@@ +1355,5 @@
> +    }
> +    mFamilyCharacterMap.Compact();
> +    mFamilyCharacterMapInitialized = true;
> +}
> +

nit: one less newline
Attachment #8363478 - Flags: review?(bas) → review+
Attachment #8363481 - Flags: review?(bas) → review+
Comment on attachment 8363482 [details] [diff] [review]
part 4 - set up async loader intrastructure with osx implementation

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

::: gfx/thebes/gfxFontInfoLoader.cpp
@@ +145,5 @@
> +    }
> +
> +    nsCOMPtr<nsIRunnable> loadEvent = new AsyncFontInfoLoader(mFontInfo);
> +
> +    mFontLoaderThread->Dispatch(loadEvent, NS_DISPATCH_NORMAL);

I'm probably missing something but do we shutdown this thread somewhere when this is not cancelled but done?

::: gfx/thebes/gfxMacPlatformFontList.mm
@@ +1041,5 @@
> +MacFontInfo::LoadFontFamilyData(const nsAString& aFamilyName)
> +{
> +    // family name ==> CTFontDescriptor
> +    NSString *famName = GetNSStringForString(aFamilyName);
> +    CFStringRef family = CFStringRef(famName);

Don't see us ever releasing this ref.

@@ +1045,5 @@
> +    CFStringRef family = CFStringRef(famName);
> +
> +    CFMutableDictionaryRef attr =
> +        CFDictionaryCreateMutable(NULL, 0, &kCFTypeDictionaryKeyCallBacks,
> +                                  &kCFTypeDictionaryValueCallBacks);

idem

@@ +1047,5 @@
> +    CFMutableDictionaryRef attr =
> +        CFDictionaryCreateMutable(NULL, 0, &kCFTypeDictionaryKeyCallBacks,
> +                                  &kCFTypeDictionaryValueCallBacks);
> +    CFDictionaryAddValue(attr, kCTFontFamilyNameAttribute, family);
> +    CTFontDescriptorRef fd = CTFontDescriptorCreateWithAttributes(attr);

idem

@@ +1049,5 @@
> +                                  &kCFTypeDictionaryValueCallBacks);
> +    CFDictionaryAddValue(attr, kCTFontFamilyNameAttribute, family);
> +    CTFontDescriptorRef fd = CTFontDescriptorCreateWithAttributes(attr);
> +    CFArrayRef matchingFonts =
> +        CTFontDescriptorCreateMatchingFontDescriptors(fd, NULL);

idem

@@ +1063,5 @@
> +    for (f = 0; f < numFaces; f++) {
> +        mLoadStats.fonts++;
> +
> +        CTFontDescriptorRef faceDesc =
> +            (CTFontDescriptorRef)CFArrayGetValueAtIndex(matchingFonts, f);

idem
Attachment #8363482 - Flags: review?(bas) → review-
Comment on attachment 8363482 [details] [diff] [review]
part 4 - set up async loader intrastructure with osx implementation

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

::: gfx/thebes/gfxFont.h
@@ +492,5 @@
>      // caller is responsible to do any sanitization/validation necessary.
>      hb_blob_t* GetTableFromFontData(const void* aFontData, uint32_t aTableTag);
>  
> +    // lookup the cmap in cached font data
> +    virtual gfxCharacterMap* GetCMAPFromFontInfo(FontInfoData *aFontInfoData,

Should this be already_AddRefed?

::: gfx/thebes/gfxFontInfoLoader.h
@@ +194,5 @@
>      };
>  
> +    // CreateFontInfo - create platform-specific object used
> +    //                  to load system-wide font info
> +    virtual FontInfoData* CreateFontInfoData() {

This should be an already_AddRefed<> since FontInfoData is refcounted.
Comment on attachment 8363483 [details] [diff] [review]
part 5 - font info loading implementation for DirectWrite

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

::: gfx/thebes/gfxDWriteFontList.cpp
@@ +1725,5 @@
> +        }
> +    }
> +}
> +
> +FontInfoData*

This needs to be already_AddRefed as well then.
Attachment #8363483 - Flags: review?(bas) → review+
Comment on attachment 8364210 [details] [diff] [review]
part 6 - font info loading for GDI

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

::: gfx/thebes/gfxFont.cpp
@@ +1369,5 @@
>                      nameTable, gfxFontUtils::NAME_ID_POSTSCRIPT, psname) == NS_OK)
>              {
>                  aPlatformFontList->AddPostscriptName(fe, psname);
>              }
> +       }

nit: probably not the best patch to have this change in, although I don't feel strongly

::: gfx/thebes/gfxGDIFontList.cpp
@@ +1087,5 @@
> +        mLoadStats.othernames += data.mOtherFamilyNames.Length();
> +    }
> +}
> +
> +FontInfoData*

again, already_AddRefed
Attachment #8364210 - Flags: review?(bas) → review+
Assignee

Comment 16

6 years ago
(In reply to Bas Schouten (:bas.schouten) from comment #12)
> Comment on attachment 8363482 [details] [diff] [review]
> part 4 - set up async loader intrastructure with osx implementation
> 
> Review of attachment 8363482 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxFontInfoLoader.cpp
> @@ +145,5 @@
> > +    }
> > +
> > +    nsCOMPtr<nsIRunnable> loadEvent = new AsyncFontInfoLoader(mFontInfo);
> > +
> > +    mFontLoaderThread->Dispatch(loadEvent, NS_DISPATCH_NORMAL);
> 
> I'm probably missing something but do we shutdown this thread somewhere when
> this is not cancelled but done?

Threads are automatically shutdown when no more events exist and there
are no longer any references. I verified that the thread goes away
after the font loader completes.

> ::: gfx/thebes/gfxMacPlatformFontList.mm
> @@ +1041,5 @@
> > +MacFontInfo::LoadFontFamilyData(const nsAString& aFamilyName)
> > +{
> > +    // family name ==> CTFontDescriptor
> > +    NSString *famName = GetNSStringForString(aFamilyName);
> > +    CFStringRef family = CFStringRef(famName);
> 
> Don't see us ever releasing this ref.

This string is created via NSString so it's handled by the NSAutoReleasePool.

> @@ +1063,5 @@
> > +    for (f = 0; f < numFaces; f++) {
> > +        mLoadStats.fonts++;
> > +
> > +        CTFontDescriptorRef faceDesc =
> > +            (CTFontDescriptorRef)CFArrayGetValueAtIndex(matchingFonts, f);
> 
> idem

The array holds the reference, so there's no need to explicitly
release faceDesc.
Assignee

Comment 17

6 years ago
Updated based on review comments.
Attachment #8363477 - Attachment is obsolete: true
Attachment #8364815 - Flags: review?(bas)
Assignee

Comment 18

6 years ago
Updated based on review comments.
Attachment #8363482 - Attachment is obsolete: true
Attachment #8364816 - Flags: review?(bas)
This sounds cool.

(In reply to John Daggett (:jtd) from comment #0)
> The interval timer to transfer font data back to the platform font
> list generally isn't needed; since the transfer is basically just
> swizzling around data structures, the transfer will usually occur
> within the initial allotted time slice. For systems with huge numbers
> of fonts (i.e. >3000 fonts) or in cases where simply enumerating fonts
> takes a long time (due to overzealous virus-checker code for example)
> it may take longer and the interval timer will fire multiple times.

If this hardly ever runs, how are we going to test it? Should we set the timer value to some especially low value when running tests to ensure this code is tested?
Comment on attachment 8364815 [details] [diff] [review]
part 1 v2, use CTFontManager API to enumerate font families in OSX

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

::: gfx/thebes/gfxMacPlatformFontList.mm
@@ +686,4 @@
>  
> +    numFamilies = CFArrayGetCount(familyNames);
> +    for (i = 0; i < numFamilies; i++) {
> +        CFStringRef family = (CFStringRef)CFArrayGetValueAtIndex(familyNames, i);

I think you're correctly not releasing this.. but I wish I could find documentation proving that.
Attachment #8364815 - Flags: review?(bas) → review+
(In reply to Bas Schouten (:bas.schouten) from comment #20)
> Comment on attachment 8364815 [details] [diff] [review]
> part 1 v2, use CTFontManager API to enumerate font families in OSX
> 
> Review of attachment 8364815 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxMacPlatformFontList.mm
> @@ +686,4 @@
> >  
> > +    numFamilies = CFArrayGetCount(familyNames);
> > +    for (i = 0; i < numFamilies; i++) {
> > +        CFStringRef family = (CFStringRef)CFArrayGetValueAtIndex(familyNames, i);
> 
> I think you're correctly not releasing this.. but I wish I could find
> documentation proving that.

It's standard Core Foundation ownership policy: "Create" and "Copy" APIs give you a retained (owning) reference that you have to release; "Get" APIs give you a non-retained reference and therefore must *not* be released by the caller (unless you explicitly Retain the object after Getting it).

https://developer.apple.com/library/mac/documentation/CoreFOundation/Conceptual/CFMemoryMgmt/Concepts/Ownership.html#//apple_ref/doc/uid/20001148-SW1
Comment on attachment 8364816 [details] [diff] [review]
part 4 v2 - set up async loader intrastructure with osx implementation

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

I may not have done the most thorough job reviewing this because I'm not -that- familiar with the code, but it looks pretty good now to me.

::: gfx/thebes/gfxMacPlatformFontList.mm
@@ +240,5 @@
>      } else {
> +        uint32_t kCMAP = TRUETYPE_TAG('c','m','a','p');
> +        charmap = new gfxCharacterMap();
> +        AutoTable cmapTable(this, kCMAP);
> +    

nit: whitespace

@@ +1056,5 @@
> +        CTFontDescriptorCreateMatchingFontDescriptors(fd, NULL);
> +    if (!matchingFonts) {
> +        return;
> +    }
> +    CFRelease(fd);

move this above the if clause so it gets executed even if !matchingFonts

@@ +1067,5 @@
> +    for (f = 0; f < numFaces; f++) {
> +        mLoadStats.fonts++;
> +
> +        CTFontDescriptorRef faceDesc =
> +            (CTFontDescriptorRef)CFArrayGetValueAtIndex(matchingFonts, f);

same as in the last review comment :s
Attachment #8364816 - Flags: review?(bas) → review+
Assignee

Comment 23

6 years ago
(In reply to Bas Schouten (:bas.schouten) from comment #22)
> Comment on attachment 8364816 [details] [diff] [review]
> part 4 v2 - set up async loader intrastructure with osx implementation
> 
> Review of attachment 8364816 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I may not have done the most thorough job reviewing this because I'm not
> -that- familiar with the code, but it looks pretty good now to me.

Thanks so much!

> @@ +1067,5 @@
> > +    for (f = 0; f < numFaces; f++) {
> > +        mLoadStats.fonts++;
> > +
> > +        CTFontDescriptorRef faceDesc =
> > +            (CTFontDescriptorRef)CFArrayGetValueAtIndex(matchingFonts, f);
> 
> same as in the last review comment :s

These are not explicitly released, see comment 21.
Assignee

Comment 24

6 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> This sounds cool.
> 
> (In reply to John Daggett (:jtd) from comment #0)
> > The interval timer to transfer font data back to the platform font
> > list generally isn't needed; since the transfer is basically just
> > swizzling around data structures, the transfer will usually occur
> > within the initial allotted time slice. For systems with huge numbers
> > of fonts (i.e. >3000 fonts) or in cases where simply enumerating fonts
> > takes a long time (due to overzealous virus-checker code for example)
> > it may take longer and the interval timer will fire multiple times.
> 
> If this hardly ever runs, how are we going to test it? Should we set the
> timer value to some especially low value when running tests to ensure this
> code is tested?

Under OSX the interval timer runs anytime the disabled-by-default pref
gfx.font_rendering.fallback.always_use_cmaps pref is enabled, since
this forces cmap loading for all fonts.  Under OSX there's still a
bunch of stuff we do in this case on the main thread to deal with
support for complex scripts, so in that case the interval timer will
fire. On older systems running XP, I imagine this will be hit in cold
startup situations where simply enumerating fonts takes longer than
usual.

Not sure setting the timer low is such a great idea unless you mean
strictly limited to some sort of environment in which tests run (e.g.
for reftest/mochitest runs for example).  The interval is set by
gfx.font_loader.interval so that's easy enough to do.

Comment 27

5 years ago
I landed a trivial wchar_t/char16_t mismatch fixup for mingw:

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb348e535e65

Updated

2 years ago
Blocks: 491283
You need to log in before you can comment on or make changes to this bug.