Bug 195208 - [ThreadedCompositor] Simplify the compositing run loop worker thread
Summary: [ThreadedCompositor] Simplify the compositing run loop worker thread
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk, InRadar
: 186444 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-03-01 06:54 PST by Carlos Garcia Campos
Modified: 2019-03-04 05:58 PST (History)
5 users (show)

See Also:


Attachments
Patch (7.58 KB, patch)
2019-03-01 06:57 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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).