[webkit-reviews] review granted: [Bug 233747] move FontCache singleton and instance on WorkerGlobalScope to ThreadGlobalData : [Attachment 445673] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Cameron McCormack (:heycam)
<heycam at apple.com>'s request for review:
Bug 233747: move FontCache singleton and instance on WorkerGlobalScope to
ThreadGlobalData
https://bugs.webkit.org/show_bug.cgi?id=233747

Attachment 445673: Patch

https://bugs.webkit.org/attachment.cgi?id=445673&action=review




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 445673
  --> https://bugs.webkit.org/attachment.cgi?id=445673
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445673&action=review

What’s the performance impact of fetching thread global data in all these
places, where many were accessing a local variable or data member before?

> Source/WebCore/page/SettingsBase.cpp:61
> +    // FIXME: Call invalidateFontCascadeCache() on all workers.
> +    FontCache::forCurrentThread().invalidateFontCascadeCache();

Can we do this through a FontCache::forEachFontCache function, analogous to
Page::forEachPage, or do we need to keep the registry of "all" somewhere
outside the FontCache class?

One reason I ask is that if we know we are going to use that interface, we
could deploy it now while touching each of these call sites even if right now
it only did the current thread one. Then we’d have fewer FIXME in this patch.

> Source/WebCore/platform/ThreadGlobalData.cpp:68
> +    if (m_fontCache)
> +	   m_fontCache->invalidate();

What destroys the font cache? Does calling invalidate() also null out
m_fontCache?

> Source/WebCore/platform/graphics/FontCache.h:286
> +    static std::unique_ptr<FontCache> create();

Not sure we need this function. Typically we just make the constructor public
and use make_unique directly for non-reference-counted things.

Or we could try to keep things private and use friend to make it accessible to
ThreadGlobalData. Could be the create function or the constructor.

> Source/WebKit/WebProcess/WebProcess.cpp:873
>  #ifndef NDEBUG
>      GCController::singleton().garbageCollectNow();
> -    FontCache::singleton().invalidate();
> +    // FIXME: What about FontCaches on worker threads?
> +    FontCache::forCurrentThread().invalidate();
>      MemoryCache::singleton().setDisabled(true);
>  #endif

Question for whoever originally wrote this: Why is doing this work at
termination, but only in debug versions, a good idea? Lack of a comment here
makes this mysterious!