Bug 90324 – [chromium] Merge updates and idle updates into one pass

• Component: New Bugs
• Status: RESOLVED FIXED

[Eric Penner]

[chromium] Merge painting and idle painting into one pass

[Eric Penner]

Created attachment 150273 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Eric Penner]

Created attachment 150695 [details]
fix_tests

[Eric Penner]

Comment on attachment 150695 [details]
fix_tests

Fixed tests. Finally down to one paint pass!

[Eric Penner]

Created attachment 150697 [details]
rebase_just_in_case

[Dana Jansens]

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.

[Adrienne Walker]

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?

[Eric Penner]

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.

[Dana Jansens]

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.

[Eric Penner]

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).

[Eric Penner]

*** Bug 83628 has been marked as a duplicate of this bug. ***

[Eric Penner]

Created attachment 151355 [details]
Patch

[Eric Penner]

Created attachment 151365 [details]
rebase_onto_master

[Eric Penner]

Created attachment 151368 [details]
rename_layerRect

[Adrienne Walker]

(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.

[Eric Penner]

(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.

[Adrienne Walker]

Comment on attachment 151368 [details]
rename_layerRect

R=me.

[Dana Jansens]

Comment on attachment 151368 [details]
rename_layerRect

lgtm nice job

[WebKit Review Bot]

Comment on attachment 151368 [details]
rename_layerRect

Clearing flags on attachment: 151368

Committed r122185: <http://trac.webkit.org/changeset/122185>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.