Bug 158243 – Precache primary font in a secondary thread

• Status: REOPENED
• Component: Layout and Rendering

[Antti Koivisto]

Initializing system fonts is expensive. We might improve performance by moving some of the work out of the main thread.

[Antti Koivisto]

Created attachment 280195 [details]
patch

[WebKit Commit Bot]

Attachment 280195 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/FontCache.h:235:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Created attachment 280197 [details]
patch

[WebKit Commit Bot]

Attachment 280197 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/FontCache.h:235:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Created attachment 280224 [details]
patch

[WebKit Commit Bot]

Attachment 280224 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/FontCache.h:235:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Comment on attachment 280224 [details]
patch

r=me

[Antti Koivisto]

Created attachment 280231 [details]
patch

[WebKit Commit Bot]

Attachment 280231 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/FontCache.h:235:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

ttps://trac.webkit.org/r201551

[Antti Koivisto]

https://trac.webkit.org/r201551

[Chris Dumez]

Comment on attachment 280231 [details]
patch

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

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:834
> +    queue.dispatch([task = task.release()] {

[Task = WTFMove(task)]() mutable

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:840
> +        RunLoop::main().dispatch([task] {

Task = WTFMove(task)

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:841
> +            std::unique_ptr<PrecacheTask> deleter(task);

No need

[Brady Eidson]

I believe this patch has a number of thread safety issues.

[Brady Eidson]

(In reply to comment #13)
> I believe this patch has a number of thread safety issues.

Namely the chunk at the end of FontCacheCoreText.

At the very least there's one string thread safety issue by passing the family to the work queue not as an isolatedCopy.

[Brady Eidson]

(In reply to comment #14)
> (In reply to comment #13)
> > I believe this patch has a number of thread safety issues.
> 
> Namely the chunk at the end of FontCacheCoreText.
> 
> At the very least there's one string thread safety issue by passing the
> family to the work queue not as an isolatedCopy.

Maybe not, since the Task itself is deleted on the main thread, and an isolated copy is made in work queue.

But it's not at all obvious at a glance (hence my comment).

Anyways, I'm doing a pass over all RunLoop::dispatch users right now which is why I noticed this - I'll clean it up a bit like Chris suggested after landing.

[Ryan Haddad]

This change caused 3 API test failures on iOS-simulator

<https://build.webkit.org/builders/Apple%20iOS%209%20Simulator%20Release%20WK2%20%28Tests%29/builds/6153>

Tests that failed:
  WebCoreNSURLSessionTest.BasicOperation
  WebCoreNSURLSessionTest.InvalidateEmpty
  WebKit1.AudioSessionCategoryIOS

[Ryan Haddad]

(In reply to comment #16)
> This change caused 3 API test failures on iOS-simulator
> 
> <https://build.webkit.org/builders/
> Apple%20iOS%209%20Simulator%20Release%20WK2%20%28Tests%29/builds/6153>
> 
> Tests that failed:
>   WebCoreNSURLSessionTest.BasicOperation
>   WebCoreNSURLSessionTest.InvalidateEmpty
>   WebKit1.AudioSessionCategoryIOS

hread 3 Crashed:: Dispatch queue: org.webkit.font-precache
0   com.apple.WebCore             	0x000000010d908d9b WTF::NoncopyableFunction<void ()>::CallableWrapper<WebCore::FontCache::platformPrecache(WTF::AtomicString const&, WebCore::FontDescription const&, std::__1::function<void (std::__1::unique_ptr<WebCore::FontPlatformData, std::__1::default_delete<WebCore::FontPlatformData> >, bool)>&&)::$_0>::call() + 219 (NoncopyableFunction.h:88)
1   JavaScriptCore                	0x000000010c6f5a83 ___ZN3WTF9WorkQueue8dispatchEONS_19NoncopyableFunctionIFvvEEE_block_invoke + 35 (WorkQueueCocoa.cpp:37)
2   libdispatch.dylib             	0x00000001116fbc89 _dispatch_call_block_and_release + 12
3   libdispatch.dylib             	0x00000001117173d7 _dispatch_client_callout + 8
4   libdispatch.dylib             	0x0000000111701018 _dispatch_queue_drain + 1048
5   libdispatch.dylib             	0x00000001117009d0 _dispatch_queue_invoke + 595
6   libdispatch.dylib             	0x00000001117022e8 _dispatch_root_queue_drain + 565
7   libdispatch.dylib             	0x00000001117020ac _dispatch_worker_thread3 + 98
8   libsystem_pthread.dylib       	0x0000000111a4f4de _pthread_wqthread + 1129
9   libsystem_pthread.dylib       	0x0000000111a4d341 start_wqthread + 13

[WebKit Commit Bot]

Re-opened since this is blocked by bug 158275

[Chris Dumez]

Looks like this was a 0.7% regression on PLT on MacBookPro.