[chromium] Merge painting and idle painting into one pass
Created attachment 150273 [details] Patch
Comment on attachment 150273 [details] Patch Attachment 150273 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13106659 New failing tests: TiledLayerChromiumTest.tilesNotPaintedWithoutInvalidation TiledLayerChromiumTest.idlePaintNonVisibleAnimatingLayers
Created attachment 150306 [details] Archive of layout-test-results from gce-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-12.04-precise
Created attachment 150695 [details] fix_tests
Comment on attachment 150695 [details] fix_tests Fixed tests. Finally down to one paint pass!
Created attachment 150697 [details] rebase_just_in_case
Comment on attachment 150697 [details] rebase_just_in_case View in context: https://bugs.webkit.org/attachment.cgi?id=150697&action=review > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:633 > IntRect idlePaintLayerRect = idlePaintRect(layerRect); While we're here, can we rename layerRect in this method to contentRect (since this rect is in content space)? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:495 > + m_needMoreIdleUpdates = false; This new member var is only read here, and written inside paintLayerContents. How about making paintLayerContents (and paintMasksForRenderSurface) return a bool instead? Then we can avoid adding a member variable to CCLTH. > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:-607 > - // Non-visible layers don't idle paint. > - layer->idleUpdateLayerRect(updater, contentRect, 0); > - I'm not so sure about the TLC test changes such as this. Isn't just removing all these calls just avoiding what these are trying to test? Should we instead be calling updateLayerRect() in all these cases, since idleUpdate has been mereged into it? Or can we restructure these tests in some way that they are still verifying the things they used to be? > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:-1442 > - // No metrics are recorded in prepaint, so the values should not change from above. Do you intend to make painting metrics recorded during prepaint now? This will change the meaning of the metrics. I think we shouldn't do that, and if we want to record metrics, it should record them separately from non-prepainting.
Comment on attachment 150697 [details] rebase_just_in_case View in context: https://bugs.webkit.org/attachment.cgi?id=150697&action=review This patch is super exciting to see. > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:91 > + // After preparing an update, returns true if more painting is needed. > bool needsIdlePaint(const IntRect& layerRect); Also while we're here, is there a reason needsIdlePaint takes a parameter? If it always takes the visibleRect, maybe it should just do that itself, internally?
Comment on attachment 150697 [details] rebase_just_in_case View in context: https://bugs.webkit.org/attachment.cgi?id=150697&action=review >> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:633 >> IntRect idlePaintLayerRect = idlePaintRect(layerRect); > > While we're here, can we rename layerRect in this method to contentRect (since this rect is in content space)? SG >> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:91 >> bool needsIdlePaint(const IntRect& layerRect); > > Also while we're here, is there a reason needsIdlePaint takes a parameter? If it always takes the visibleRect, maybe it should just do that itself, internally? I think that will break tests because the tests often don't set the visible layer rect. All other methods take an extra parameter (like a utility function). I'm kind of inclined to agree with you and remove the parameter from all functions that just use the visible rect... But some test cleanup will need to go with it. >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:495 >> + m_needMoreIdleUpdates = false; > > This new member var is only read here, and written inside paintLayerContents. > > How about making paintLayerContents (and paintMasksForRenderSurface) return a bool instead? Then we can avoid adding a member variable to CCLTH. SG >> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:-607 >> - > > I'm not so sure about the TLC test changes such as this. Isn't just removing all these calls just avoiding what these are trying to test? Should we instead be calling updateLayerRect() in all these cases, since idleUpdate has been mereged into it? Or can we restructure these tests in some way that they are still verifying the things they used to be? Keep in mind that before only one of updateLayerRect and idleUpdateLayerRect would actually do something. Now the next updateLayerRect will take the place of the previous idleUpdateLayerRect. I checked each case to insure that this makes sense. >> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:-1442 >> - // No metrics are recorded in prepaint, so the values should not change from above. > > Do you intend to make painting metrics recorded during prepaint now? This will change the meaning of the metrics. I think we shouldn't do that, and if we want to record metrics, it should record them separately from non-prepainting. Hmm. I'll need to look closer at how the metrics are segmented. I didn't intend it to now records pre-paint as part of paint, but rather than this code didn't make sense to me in light of merging the two passes.
Comment on attachment 150697 [details] rebase_just_in_case View in context: https://bugs.webkit.org/attachment.cgi?id=150697&action=review >>> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:-607 >>> - >> >> I'm not so sure about the TLC test changes such as this. Isn't just removing all these calls just avoiding what these are trying to test? Should we instead be calling updateLayerRect() in all these cases, since idleUpdate has been mereged into it? Or can we restructure these tests in some way that they are still verifying the things they used to be? > > Keep in mind that before only one of updateLayerRect and idleUpdateLayerRect would actually do something. Now the next updateLayerRect will take the place of the previous idleUpdateLayerRect. I checked each case to insure that this makes sense. Ok, well I commented on this one in particular because it looks odd atm. We check numPaintedTiles() after this call to ensure it's 0. If we're not making this call, that check isn't needed, nor anything that follows in this test. Other tests look similar to me also, so I think there's a bit more cleanup to be done at least.
Comment on attachment 150697 [details] rebase_just_in_case View in context: https://bugs.webkit.org/attachment.cgi?id=150697&action=review >>>> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:-607 >>>> - >>> >>> I'm not so sure about the TLC test changes such as this. Isn't just removing all these calls just avoiding what these are trying to test? Should we instead be calling updateLayerRect() in all these cases, since idleUpdate has been mereged into it? Or can we restructure these tests in some way that they are still verifying the things they used to be? >> >> Keep in mind that before only one of updateLayerRect and idleUpdateLayerRect would actually do something. Now the next updateLayerRect will take the place of the previous idleUpdateLayerRect. I checked each case to insure that this makes sense. > > Ok, well I commented on this one in particular because it looks odd atm. We check numPaintedTiles() after this call to ensure it's 0. If we're not making this call, that check isn't needed, nor anything that follows in this test. Other tests look similar to me also, so I think there's a bit more cleanup to be done at least. Ahh I see! In most cases they were always grouped together with updateLayerRect being first. I'll make sure there is always at least one call in each case. >>> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:-1442 >>> - // No metrics are recorded in prepaint, so the values should not change from above. >> >> Do you intend to make painting metrics recorded during prepaint now? This will change the meaning of the metrics. I think we shouldn't do that, and if we want to record metrics, it should record them separately from non-prepainting. > > Hmm. I'll need to look closer at how the metrics are segmented. I didn't intend it to now records pre-paint as part of paint, but rather than this code didn't make sense to me in light of merging the two passes. I'll double check that we aren't corrupting our metrics and verify that here (rather than just removing this).
*** Bug 83628 has been marked as a duplicate of this bug. ***
Created attachment 151355 [details] Patch
Created attachment 151365 [details] rebase_onto_master
Created attachment 151368 [details] rename_layerRect
(In reply to comment #11) > (From update of attachment 150697 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150697&action=review > > > Hmm. I'll need to look closer at how the metrics are segmented. I didn't intend it to now records pre-paint as part of paint, but rather than this code didn't make sense to me in light of merging the two passes. > > I'll double check that we aren't corrupting our metrics and verify that here (rather than just removing this). What was the result of this investigation? (In reply to comment #9) > (From update of attachment 150697 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150697&action=review > > >> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:91 > >> bool needsIdlePaint(const IntRect& layerRect); > > > > Also while we're here, is there a reason needsIdlePaint takes a parameter? If it always takes the visibleRect, maybe it should just do that itself, internally? > > I think that will break tests because the tests often don't set the visible layer rect. All other methods take an extra parameter (like a utility function). I'm kind of inclined to agree with you and remove the parameter from all functions that just use the visible rect... But some test cleanup will need to go with it. I think there may have been some historical reason that all the TiledLayerChromium functions take the visible layer rect. I think they should just be removed all in one go in a follow-up patch, and this should just get landed first.
(In reply to comment #16) > What was the result of this investigation? The metrics are controlled by passing the CCOcclusionTracker to TiledLayerChromium::updateTiles. Since the painting and idle painting steps are merged into one functions now (not easy to test the occlusion tracking results independently), I added an ASSERT in TiledLayerChromium::updateTiles to insure we never track metrics for idle painting. > I think there may have been some historical reason that all the TiledLayerChromium functions take the visible layer rect. I think they should just be removed all in one go in a follow-up patch, and this should just get landed first. I think it would be a bigger change now yeah. I did change the layerRect name in this CL though (as per Dana's suggestion), as that wasn't as big of a change.
Comment on attachment 151368 [details] rename_layerRect R=me.
Comment on attachment 151368 [details] rename_layerRect lgtm nice job
Comment on attachment 151368 [details] rename_layerRect Clearing flags on attachment: 151368 Committed r122185: <http://trac.webkit.org/changeset/122185>
All reviewed patches have been landed. Closing bug.