Currently, QQuickWebPage::updatePaintNode() calls LayerTreeRenderer::syncRemoteContent() with locking the main thread. syncRemoteContent() is heavy, so we should not call syncRemoteContent() in updatePaintNode() because syncRemoteContent() does not access any data structures, which the main thread use. After this patch, syncRemoteContent() is used only internally, so syncRemoteContent() becomes private.
Created attachment 175376 [details] Patch
After this patch, I can feel reducing lag due to texture uploading. For example, snowstack. http://www.satine.org/research/webkit/snowleopard/snowstack.html
I decided to post this patch after reading Slava's mail. Thank Slava for explanation! Qt WK2 port already runs layer tree painting on separate thread. Layer tree renderer is created on main thread, but rendering is performed only on paint thread. Qt5 creates paint nodes for painting and QQuickWebPage paint node keeps threaded reference on LayerTreeRenderer. Messages to LayerTreeRenderer are delivered through dispatchUpdate/bind/renderQueue . All updates from renderQueue to renderer are applied in QQuickWebPage::updatePaintNode call. QQuickItem::updatePaintNode is very special call. During this call main thread is locked and it is safe to access main thread objects from paint thread. http://doc-snapshot.qt-project.org/5.0/qquickitem.html#updatePaintNode So, the only thing that needs to be implemented for GTK is replacement for Qt paint node and sync point similar to QQuickItem::updatePaintNode .
Comment on attachment 175376 [details] Patch Good catch!
Comment on attachment 175376 [details] Patch Clearing flags on attachment: 175376 Committed r135401: <http://trac.webkit.org/changeset/135401>
All reviewed patches have been landed. Closing bug.
Comment on attachment 175376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175376&action=review > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:125 > + syncRemoteContent(); Actually, I doubt that it improves something and I would suspect regression here. QQuickWebPage::updatePaintNode is called only when update is required (like new tiles coming). LayerTreeRenderer::paintToCurrentGLContext is called for every paint, so with this patch it will be called more often and that's why individual tiles might come faster, because paint node update is called only from didRenderFrame. If faster displaying of incoming tile updates is desired this could be achieved by calling updateViewPort more often in LayerTreeCoordinatorProxy (like, for every incoming tile update), but this could cause visual partial update artifacts. Also, syncRemoteContent originally was also called from paint and I removed it intentionally because this function uses renderQueue which is accessible both from main and paint threads. Previously renderQueue had no locking and it worked because Qt5 guarantees that main thread is locked during updatePaintNode call. Now this might work because of locking of renderQueue, but it will crash it locking would be removed.
(In reply to comment #7) > (From update of attachment 175376 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175376&action=review > > > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:125 > > + syncRemoteContent(); > > Actually, I doubt that it improves something and I would suspect regression here. Thanks for helpful explanation! I could really feel reducing lag due to texture uploading. > QQuickWebPage::updatePaintNode is called only when update is required (like new tiles coming). LayerTreeRenderer::paintToCurrentGLContext is called for every paint, so with this patch it will be called more often and that's why individual tiles might come faster, because paint node update is called only from didRenderFrame. If faster displaying of incoming tile updates is desired this could be achieved by calling updateViewPort more often in LayerTreeCoordinatorProxy (like, for every incoming tile update), but this could cause visual partial update artifacts. Before I had changed, I hesitated due to your reason: updatePaintNode is called if needed. After taking some time to think, I arrived at the conclusion: it is safe. It is because we update tiles when handling didRenderFrame in that calling flushLayerChanges(), especially commitTileOperations(). LayerTreeRenderer::paintToCurrentGLContext can causes calling updateTile() but actual update is deferred until flushLayerChanges() is called. Furthermore, Web Process still wait to progress until receiving a RenderNextFrame message. void LayerTreeRenderer::flushLayerChanges() { m_renderedContentsScrollPosition = m_pendingRenderedContentsScrollPosition; // Since the frame has now been rendered, we can safely unlock the animations until the next layout. setAnimationsLocked(false); m_rootLayer->flushCompositingState(FloatRect()); commitTileOperations(); removeReleasedImageBackingsIfNeeded(); // The pending tiles state is on its way for the screen, tell the web process to render the next one. dispatchOnMainThread(bind(&LayerTreeRenderer::renderNextFrame, this)); } > Also, syncRemoteContent originally was also called from paint and I removed it intentionally because this function uses renderQueue which is accessible both from main and paint threads. Previously renderQueue had no locking and it worked because Qt5 guarantees that main thread is locked during updatePaintNode call. Now this might work because of locking of renderQueue, but it will crash it locking would be removed. Yes, your work ensuring atomic operation encouraged me to make the patch. syncRemoteContent is not called from external clients anymore and LayerTreeRenderer::paintToGraphicsContext also calls syncRemoteContent, so I think it is natural for LayerTreeRenderer::paintToCurrentGLContext to call syncRemoteContent now. However, when another requirement exists, syncRemoteContent can be public again. I'm ok. Please let me know if there is gap with your opinion.