Initializing system fonts is expensive. We might improve performance by moving some of the work out of the main thread.
Created attachment 280195 [details] patch
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.
Created attachment 280197 [details] patch
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.
Created attachment 280224 [details] patch
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 on attachment 280224 [details] patch r=me
Created attachment 280231 [details] patch
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.
ttps://trac.webkit.org/r201551
https://trac.webkit.org/r201551
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
I believe this patch has a number of thread safety issues.
(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.
(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.
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
(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
Re-opened since this is blocked by bug 158275
Looks like this was a 0.7% regression on PLT on MacBookPro.