Summary: | [Threaded Compositor] Crashes and deadlocks in single web process mode | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bugs-noreply, commit-queue, zan | ||||||||||||
Priority: | P2 | Keywords: | Gtk | ||||||||||||
Version: | WebKit Local Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 154066 | ||||||||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2016-07-25 03:14:15 PDT
Created attachment 284470 [details]
Patch
Created attachment 284472 [details]
Updated patch
I forgot to detach the scene on invalidate.
Created attachment 284476 [details]
New patch
I've realized of an issue with the previous patch, the sync tasks should be sync for the threaded compositor, not for the compositing run loop. After moving the performSync method to the ThreadedCompositor class the CompositingRunLoop class became a very simple RunLoop wrapper with similar code to what a WorkQueue is. so this patch removes CompositingRunLoop in favor of using a global WorkQueue.
Attachment 284476 [details] did not pass style-queue:
ERROR: Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.h:93: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:71: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 2 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Any additional information on why this is occurring in the first place? I don't think CompositingRunLoop should be removed, instead it should be made a singleton for now, and later it could be turned into a thread pool. (In reply to comment #5) > Any additional information on why this is occurring in the first place? I don't think we can use X and most of the graphics drivers from different processes. > I don't think CompositingRunLoop should be removed, instead it should be > made a singleton for now, and later it could be turned into a thread pool. That's what my previous patch did, converted it into a singleton, but after moving the performSync to the threaded compositor the class became just a wrapper around RunLoop in a separate thread which is what WorkQueue does in the end. I would do it the other way around, let's bring it back if it's really possible to use multiple threads at some point, because I don't think that's going to happen. (In reply to comment #7) > I would do it the other way around, let's bring it back if it's really > possible to use multiple threads at some point, because I don't think that's > going to happen. It's possible with non-X11 setups. (In reply to comment #6) > (In reply to comment #5) > > Any additional information on why this is occurring in the first place? > > I don't think we can use X and most of the graphics drivers from different > processes. > I don't understand this. (In reply to comment #9) > (In reply to comment #6) > > (In reply to comment #5) > > > Any additional information on why this is occurring in the first place? > > > > I don't think we can use X and most of the graphics drivers from different > > processes. > > > > I don't understand this. That I think we will never be able to use multiple threads, because X and graphics drivers don't really support it. But if you say it's supported by non X platforms, I'll go back to previous approach keeping the CompositingRunLoop because I need this fixed. Created attachment 284483 [details]
New patch
Bring back the compositing run loop class, making it a thread pool with a limit of 1 thread by default.
Attachment 284483 [details] did not pass style-queue:
ERROR: Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:65: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.h:92: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/CompositingRunLoop.h:50: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/CompositingRunLoop.cpp:69: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 4 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 284569 [details]
Another approach
Updated the patch after talking to Zan using a slightly different approach.
Attachment 284569 [details] did not pass style-queue:
ERROR: Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/CompositingRunLoop.cpp:49: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 1 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 284569 [details] Another approach View in context: https://bugs.webkit.org/attachment.cgi?id=284569&action=review Looks good, thanks for reiterating on this. > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/CompositingRunLoop.cpp:94 > + WorkQueue& getOrCreateWorkQueueForContext(void* context) > + { > + auto addResult = m_workQueueMap.add(context, nullptr); > + if (addResult.isNewEntry) { > + if (m_threadCount >= m_threadCountLimit) { > + RELEASE_ASSERT(m_sharedWorkQueue); > + addResult.iterator->value = m_sharedWorkQueue; > + } else { > + addResult.iterator->value = WorkQueue::create("org.webkit.ThreadedCompositorWorkQueue"); > + if (!m_threadCount) > + m_sharedWorkQueue = addResult.iterator->value; > + m_threadCount++; > + } > + } This is OK for now, and it works for a single-thread limit. But for configurations where more (but not unlimited) threads could be used, one option would be to use a HashSet here and disperse the contexts across the available threads. But that's for the future. Committed r203718: <http://trac.webkit.org/changeset/203718> |