We always append m_layerState to coordinator in CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly. This causes renderNext-circular calls continuously. So this patch checks whether the layer needs to be updated or not, if we need to update the layer, then we append layer state to coordinator. This will reduce the needless update and IPC calls.
Created attachment 192633 [details] Proposal patch
Comment on attachment 192633 [details] Proposal patch View in context: https://bugs.webkit.org/attachment.cgi?id=192633&action=review > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:750 > + bool needToSyncThisLayer = hasChangedLayerState(); great! how about early return here or line 742?
Comment on attachment 192633 [details] Proposal patch View in context: https://bugs.webkit.org/attachment.cgi?id=192633&action=review Please correct nitpicks, otherwise patch looks ok. > Source/WebCore/ChangeLog:14 > + We always append m_layerState to coordinator in CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly. > + This causes renderNext-circular calls continuously. > + So this patch checks whether the layer needs to be updated or not, > + if we need to update the layer, then we append layer state to coordinator. > + This will reduce the needless update and IPC calls. > + > + No new tests (OOPS!). CoordinatedGraphicsLayer always sets the layer state, which results in an infinite commit/renderNextFrame loop between the web process in the UI process. This patch breaks the infinite loop by avoiding commit if the layer state hasn't changed. There is no current facility for automatically testing this, however it doesn't break existing tests. > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:750 > + bool needToSyncThisLayer = hasChangedLayerState(); bool layerNeedsSync = needsSync(); > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:775 > +bool CoordinatedGraphicsLayer::hasChangedLayerState() rename to needsSync()
Thanks for reporting. It is the bug I made. :( I think this patch can be better using CoordinatedGraphicsLayerState::changeMask ex) if (!m_layerState.changeMask) return; in CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly() or CoordinatedLayerTreeHost::syncLayerState BTW, I think we need clean up m_shouldSync...series in CoordinatedGraphicsLayerState. We can use LayerState::changeMask instead of that.
(In reply to comment #4) > Thanks for reporting. > It is the bug I made. :( > > I think this patch can be better using CoordinatedGraphicsLayerState::changeMask > > ex) > if (!m_layerState.changeMask) > return; This won't work since you also need to deal with tiles, which don't appear in the changeMask. I think JungJik's solution is ok for now.
Comment on attachment 192633 [details] Proposal patch View in context: https://bugs.webkit.org/attachment.cgi?id=192633&action=review >>> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:750 >>> + bool needToSyncThisLayer = hasChangedLayerState(); >> >> great! how about early return here or line 742? > > bool layerNeedsSync = needsSync(); what's mean? early return. the states will be changed inside syncFunctions.
Created attachment 192660 [details] Patch
(In reply to comment #5) > (In reply to comment #4) > > Thanks for reporting. > > It is the bug I made. :( > > > > I think this patch can be better using CoordinatedGraphicsLayerState::changeMask > > > > ex) > > if (!m_layerState.changeMask) > > return; > This won't work since you also need to deal with tiles, which don't appear in the changeMask. I think JungJik's solution is ok for now. I agree. I'll make another bug to clean up various states in CoordinatedGraphicsLayer.
Comment on attachment 192660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192660&action=review > Source/WebCore/ChangeLog:14 > + (WebCore::CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly): if hasChangedLayerState is true, append m_layerState. hasChangedLayerState -> needsSync > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h:179 > + bool needsSync(); This function should be const.
Created attachment 192705 [details] Patch
Created attachment 192714 [details] Patch
(In reply to comment #11) > Created an attachment (id=192714) [details] > Patch Add m_movingVisibleRect flag to needsSync.
Comment on attachment 192714 [details] Patch This is actually wrong. We might have a m_movingVisibleRect and still not need to sync. This patch would make it so that during a transform animation we'd be constantly syncing. The right approach for this is close to what ryumiel was suggesting - add an isEmpty() function to GraphicsLayerState and not sync if that returns true.
(In reply to comment #13) > (From update of attachment 192714 [details]) > This is actually wrong. We might have a m_movingVisibleRect and still not need to sync. This patch would make it so that during a transform animation we'd be constantly syncing. > The right approach for this is close to what ryumiel was suggesting - add an isEmpty() function to GraphicsLayerState and not sync if that returns true. I am not sure that ryumiel was suggesting. but I think we still need to sync when m_movingVisibleRect is true. because when m_movingVisibleRect is true, we get current transform to send UI Process. maybe the position of m_movingVisibleRect in needsSync is not correct. it would be "if (layerNeedsSync || hasActiveTransformAnimation)".
Created attachment 193074 [details] Patch
(In reply to comment #15) > Created an attachment (id=193074) [details] > Patch it's great. could you test https://bugs.webkit.org/show_bug.cgi?id=112114 with the patch? And please, ignore the description of the test. it's wrong. thank you.
(In reply to comment #16) > (In reply to comment #15) > > Created an attachment (id=193074) [details] [details] > > Patch > > it's great. could you test https://bugs.webkit.org/show_bug.cgi?id=112114 with the patch? And please, ignore the description of the test. it's wrong. thank you. Seems ok.
Comment on attachment 193074 [details] Patch r=me
Comment on attachment 193074 [details] Patch Clearing flags on attachment: 193074 Committed r145806: <http://trac.webkit.org/changeset/145806>
All reviewed patches have been landed. Closing bug.