Bug 93059 – [chromium] Refactor tile flags.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Eric Penner]

[chromium] Refactor loops to guarantee 'updated' flag correctness.

[Eric Penner]

Created attachment 156236 [details]
Patch

[Adrienne Walker]

Comment on attachment 156236 [details]
Patch

Sorry to be grumpy about this, but TiledLayerChromium is already brittle and overly complex and I don't want yet another flag.  I've seen too many bugs where somebody forgets to set or unset some of these flags.  For example, you don't set this flag to false in resetUpdateState, so it's stale on any tile that got updated last frame but not this frame.  If somebody ever touches the way these loops works, it seems possible that could bite them.  I point out resetUpdateState not because I think fixing it will make this approach palatable, but because I want to point out how easy it is to have a flag in a wrong state unexpectedly.

Is it possible to not add another flag and instead push all the complexity into the uncommon case of OOMing? Looping through all the tiles there and unsetting the update flag seems like a much more simple solution.

[Eric Penner]

I get why you are grumpy about these flags causing problems. I ran into a lot of issues with this myself on m18. I'd like to point out that I intentionally didn't reset the temp flag, because it wouldn't do any good (since again, the function can exit early).

Your idea sounds good, albeit with possibly yet another loop in this function, but maybe it will be more clear. Will post something like that soon.

[Eric Penner]

Created attachment 156757 [details]
Patch

[Eric Penner]

Created attachment 156761 [details]
rebase

[WebKit Review Bot]

Comment on attachment 156761 [details]
rebase

Attachment 156761 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13447584

New failing tests:
animations/suspend-resume-animation-events.html

[WebKit Review Bot]

Created attachment 156784 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Eric Penner]

Created attachment 156812 [details]
rebase_plus_extra_asserts

[Eric Penner]

> 
> New failing tests:
> animations/suspend-resume-animation-events.html

This is passing for me locally:
new-run-webkit-tests --debug LayoutTests/animations/suspend-resume-animation-events.html

Not sure what happened there.

[Dana Jansens]

Comment on attachment 156812 [details]
rebase_plus_extra_asserts

View in context: https://bugs.webkit.org/attachment.cgi?id=156812&action=review

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:411
> +                continue;

unneeded continue at the end of the loop

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:448
> +IntRect TiledLayerChromium::markUpdatedTiles(int left, int top, int right, int bottom, bool useOcclusions)

naming nit: markTilesForUpdate?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:473
> +    if (!paintRect.isEmpty())

I'm not sure about ifing on this, maybe recording a 0x0 paint is useful information for us? (i know overdraw metrics wont actually record anything now, but is this logic we want baked into TLC?)

[Adrienne Walker]

Comment on attachment 156812 [details]
rebase_plus_extra_asserts

View in context: https://bugs.webkit.org/attachment.cgi?id=156812&action=review

I like this a lot in general.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:459
> +            if (useOcclusions && tile->occluded)
> +                continue;

What about just tile->occluded here and in the other place? tile->occluded should always be valid regardless of useOcclusions.

[Eric Penner]

Comment on attachment 156812 [details]
rebase_plus_extra_asserts

View in context: https://bugs.webkit.org/attachment.cgi?id=156812&action=review

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:411
>> +                continue;
> 
> unneeded continue at the end of the loop

whoops. Thanks.

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:448
>> +IntRect TiledLayerChromium::markUpdatedTiles(int left, int top, int right, int bottom, bool useOcclusions)
> 
> naming nit: markTilesForUpdate?

sounds good.

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:459
>> +                continue;
> 
> What about just tile->occluded here and in the other place? tile->occluded should always be valid regardless of useOcclusions.

The flag should now always correctly indicate that the tile is occluded, but we sometimes want to ignore the flag (in pre-painting we paint occluded tiles anyway).

It would be better named 'ignoreOcclusions' here, but in other functions we use the CCOcclusionTracker pointer to indicate whether we should use occlusions or ignore them.

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:473
>> +    if (!paintRect.isEmpty())
> 
> I'm not sure about ifing on this, maybe recording a 0x0 paint is useful information for us? (i know overdraw metrics wont actually record anything now, but is this logic we want baked into TLC?)

This was just copying the old behavior (we did an early return right before calling this if the paintRect was empty). But I agree just give me the word and I'll remove the if unless you say not to.

[Adrienne Walker]

Comment on attachment 156812 [details]
rebase_plus_extra_asserts

View in context: https://bugs.webkit.org/attachment.cgi?id=156812&action=review

>>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:459
>>> +                continue;
>> 
>> What about just tile->occluded here and in the other place? tile->occluded should always be valid regardless of useOcclusions.
> 
> The flag should now always correctly indicate that the tile is occluded, but we sometimes want to ignore the flag (in pre-painting we paint occluded tiles anyway).
> 
> It would be better named 'ignoreOcclusions' here, but in other functions we use the CCOcclusionTracker pointer to indicate whether we should use occlusions or ignore them.

So many special cases.  :(

bool ignoreOcclusions would be much more readable.

[Eric Penner]

Comment on attachment 156812 [details]
rebase_plus_extra_asserts

View in context: https://bugs.webkit.org/attachment.cgi?id=156812&action=review

>>>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:459
>>>> +                continue;
>>> 
>>> What about just tile->occluded here and in the other place? tile->occluded should always be valid regardless of useOcclusions.
>> 
>> The flag should now always correctly indicate that the tile is occluded, but we sometimes want to ignore the flag (in pre-painting we paint occluded tiles anyway).
>> 
>> It would be better named 'ignoreOcclusions' here, but in other functions we use the CCOcclusionTracker pointer to indicate whether we should use occlusions or ignore them.
> 
> So many special cases.  :(
> 
> bool ignoreOcclusions would be much more readable.

Yep, sigh.. One alternative would be to unset the occlusion flag instead once we are done with it. I'll go with 'ignoreOcclusions' unless you say otherwise. 

I'm hoping that with all the dependencies laid out in updateTiles we can start to pick it apart.

[Dana Jansens]

Comment on attachment 156812 [details]
rebase_plus_extra_asserts

View in context: https://bugs.webkit.org/attachment.cgi?id=156812&action=review

>>>>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:459
>>>>> +                continue;
>>>> 
>>>> What about just tile->occluded here and in the other place? tile->occluded should always be valid regardless of useOcclusions.
>>> 
>>> The flag should now always correctly indicate that the tile is occluded, but we sometimes want to ignore the flag (in pre-painting we paint occluded tiles anyway).
>>> 
>>> It would be better named 'ignoreOcclusions' here, but in other functions we use the CCOcclusionTracker pointer to indicate whether we should use occlusions or ignore them.
>> 
>> So many special cases.  :(
>> 
>> bool ignoreOcclusions would be much more readable.
> 
> Yep, sigh.. One alternative would be to unset the occlusion flag instead once we are done with it. I'll go with 'ignoreOcclusions' unless you say otherwise. 
> 
> I'm hoping that with all the dependencies laid out in updateTiles we can start to pick it apart.

FWIW we could do the if here based on "idle" instead of based on occlusion tracker being present (and assert that it is present when not idle). I'm not sure if that's better, but it reduces the number of different if () conditions used.

>>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:473
>>> +    if (!paintRect.isEmpty())
>> 
>> I'm not sure about ifing on this, maybe recording a 0x0 paint is useful information for us? (i know overdraw metrics wont actually record anything now, but is this logic we want baked into TLC?)
> 
> This was just copying the old behavior (we did an early return right before calling this if the paintRect was empty). But I agree just give me the word and I'll remove the if unless you say not to.

Oh! Darn, ya I think removing it is an improvement. Thanks.

[Eric Penner]

Created attachment 157276 [details]
round_2

[Eric Penner]

I addressed all the comments, but also did some further refactoring based on the feedback and some more thought. The changelog describes the extra changes.

[Eric Penner]

Created attachment 157283 [details]
rebase

[Adrienne Walker]

Comment on attachment 157283 [details]
rebase

View in context: https://bugs.webkit.org/attachment.cgi?id=157283&action=review

R=me.

> Source/WebCore/ChangeLog:34
> +        As a last step I was going to merge LayerChromium::update() and
> +        LayerChromium::needsMoreUpdates() by having update() just return
> +        a boolean, but this proved to be a big change so I'm holding off
> +        on that. That would let us remove the m_failedPaint and get rid
> +        of needsIdlePaint() altogether.

This sounds nice, especially since needsMoreUpdates() has to do a lot of the same work that update() could already figure out.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:432
> +    for (int j = top; j <= bottom; ++j) {
> +        for (int i = left; i <= right; ++i) {
> +            UpdatableTile* tile = tileAt(i, j);
> +            ASSERT(tile); // Did setTexturePriorites get skipped?
> +            // FIXME: This should not ever be null.
> +            if (!tile)
> +                continue;

This seven line copy and pasted boilerplate makes me a little sad.

[Eric Penner]

Comment on attachment 157283 [details]
rebase

View in context: https://bugs.webkit.org/attachment.cgi?id=157283&action=review

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:432
>> +                continue;
> 
> This seven line copy and pasted boilerplate makes me a little sad.

I know, so bad. On that, I like the idea of extracting a vector of valid tiles before doing any of these boiler-plate loops. Perhaps then we would also be (even more) confident to remove the ASSERT/if(!tile) continue etc.

[Eric Penner]

Comment on attachment 157283 [details]
rebase

Tested in chrome, gtg.

[WebKit Review Bot]

Comment on attachment 157283 [details]
rebase

Rejecting attachment 157283 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13462252

[Eric Penner]

Arg! sorry I removed the 'oops'. I gotta refine my process for making these logs.

[Eric Penner]

Created attachment 157314 [details]
Patch

[Eric Penner]

Comment on attachment 157314 [details]
Patch

Fixed log, gtg.

[Adrienne Walker]

Comment on attachment 157314 [details]
Patch

R=me.  Let's try this again.

[WebKit Review Bot]

Comment on attachment 157314 [details]
Patch

Clearing flags on attachment: 157314

Committed r125114: <http://trac.webkit.org/changeset/125114>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.