Bug 195208

Summary: [ThreadedCompositor] Simplify the compositing run loop worker thread
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, don.olmstead, Hironori.Fujii, webkit-bug-importer, zan
Priority: P2 Keywords: Gtk, InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=186444
Attachments:
Description Flags
Patch none

Description Carlos Garcia Campos 2019-03-01 06:54:35 PST
We can remove the WorkQueuePool, since we never really supported more than one thread, and now that single process model non longer exists it doesn't even make sense. We can simply use a RunLoop instead of a WorkQueue so that the implementation is not specific to the generic WorkQueue implementation.
Comment 1 Carlos Garcia Campos 2019-03-01 06:57:44 PST
Created attachment 363328 [details]
Patch
Comment 2 WebKit Commit Bot 2019-03-01 10:08:05 PST
Comment on attachment 363328 [details]
Patch

Clearing flags on attachment: 363328

Committed r242266: <https://trac.webkit.org/changeset/242266>
Comment 3 WebKit Commit Bot 2019-03-01 10:08:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 4 Radar WebKit Bug Importer 2019-03-01 10:09:47 PST
<rdar://problem/48513504>
Comment 5 Don Olmstead 2019-03-01 10:37:15 PST
*** Bug 186444 has been marked as a duplicate of this bug. ***
Comment 6 Fujii Hironori 2019-03-03 22:29:00 PST
Comment on attachment 363328 [details]
Patch

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

> Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/CompositingRunLoop.cpp:76
> +    });

Why do you dispatch twice?
Can this code be simplified like the following?

    m_runLoop->dispatch([] {
        RunLoop::current().stop();
    });
Comment 7 Carlos Garcia Campos 2019-03-04 01:47:43 PST
Comment on attachment 363328 [details]
Patch

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

>> Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/CompositingRunLoop.cpp:76
>> +    });
> 
> Why do you dispatch twice?
> Can this code be simplified like the following?
> 
>     m_runLoop->dispatch([] {
>         RunLoop::current().stop();
>     });

No, we used to get a crash if the run loop was stopped before the timer was destroyed. So, we stop the run loop in the next main frame iteration. See bug #167545. Maybe it's no longer needed now that we don't use a WorkQueue.
Comment 8 Fujii Hironori 2019-03-04 05:58:19 PST
It seems the original issue of Bug 167545 was a race condition of:
1. Main threaed is deleting the WorkQueue
2. The WorkQueue's thread is executing a task

I think my change (comment 6) works.
I'm going to file a ticket for the change.

This code is causing a issue for WinCairo (Bug 186364 Comment 11).