Bug 158243

Summary: Precache primary font in a secondary thread
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: REOPENED ---    
Severity: Normal CC: beidson, cdumez, commit-queue, mmaxfield, ryanhaddad, simon.fraser
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 158275    
Bug Blocks:    
Attachments:
Description Flags
patch
none
patch
none
patch
kling: review+
patch none

Description Antti Koivisto 2016-05-31 15:46:57 PDT
Initializing system fonts is expensive. We might improve performance by moving some of the work out of the main thread.
Comment 1 Antti Koivisto 2016-05-31 16:12:47 PDT
Created attachment 280195 [details]
patch
Comment 2 WebKit Commit Bot 2016-05-31 16:14:51 PDT
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.
Comment 3 Antti Koivisto 2016-05-31 16:33:45 PDT
Created attachment 280197 [details]
patch
Comment 4 WebKit Commit Bot 2016-05-31 16:38:28 PDT
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.
Comment 5 Antti Koivisto 2016-06-01 02:10:03 PDT
Created attachment 280224 [details]
patch
Comment 6 WebKit Commit Bot 2016-06-01 02:12:05 PDT
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.
Comment 7 Andreas Kling 2016-06-01 05:16:32 PDT
Comment on attachment 280224 [details]
patch

r=me
Comment 8 Antti Koivisto 2016-06-01 06:48:14 PDT
Created attachment 280231 [details]
patch
Comment 9 WebKit Commit Bot 2016-06-01 06:50:31 PDT
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.
Comment 10 Antti Koivisto 2016-06-01 07:28:48 PDT
ttps://trac.webkit.org/r201551
Comment 11 Antti Koivisto 2016-06-01 07:31:44 PDT
https://trac.webkit.org/r201551
Comment 12 Chris Dumez 2016-06-01 09:08:10 PDT
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
Comment 13 Brady Eidson 2016-06-01 09:50:07 PDT
I believe this patch has a number of thread safety issues.
Comment 14 Brady Eidson 2016-06-01 09:51:32 PDT
(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.
Comment 15 Brady Eidson 2016-06-01 09:55:21 PDT
(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.
Comment 16 Ryan Haddad 2016-06-01 14:23:21 PDT
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
Comment 17 Ryan Haddad 2016-06-01 14:27:55 PDT
(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
Comment 18 WebKit Commit Bot 2016-06-01 14:34:46 PDT
Re-opened since this is blocked by bug 158275
Comment 19 Chris Dumez 2016-06-28 14:25:46 PDT
Looks like this was a 0.7% regression on PLT on MacBookPro.