Every WebPage has its own threaded compositor that runs its own compositing thread. That means that when there's more than one WebPage in the same process, we are running OpenGL stuff in different secondary threads. That's causing crashes and deadlocks in X and graphics drivers. We should ensure there's a single compositing thread per process. This is what is causing unit test WebKit2.WKPageGetScaleFactorNotZero to time out since we switched to the threaded compositor. That test is creating two pages in the same web process, and most of the times the web process crashes or deadlocks causing the test to never finish and time out.
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>