Bug 93059 - [chromium] Refactor tile flags.
Summary: [chromium] Refactor tile flags.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Penner
URL:
Keywords:
Depends on:
Blocks: 93028
  Show dependency treegraph
 
Reported: 2012-08-02 18:56 PDT by Eric Penner
Modified: 2012-08-08 16:50 PDT (History)
7 users (show)

See Also:


Attachments
Patch (6.28 KB, patch)
2012-08-02 18:59 PDT, Eric Penner
no flags Details | Formatted Diff | Diff
Patch (16.01 KB, patch)
2012-08-06 13:42 PDT, Eric Penner
no flags Details | Formatted Diff | Diff
rebase (15.97 KB, patch)
2012-08-06 14:37 PDT, Eric Penner
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-08 (564.44 KB, application/zip)
2012-08-06 16:17 PDT, WebKit Review Bot
no flags Details
rebase_plus_extra_asserts (16.36 KB, patch)
2012-08-06 17:44 PDT, Eric Penner
no flags Details | Formatted Diff | Diff
round_2 (24.25 KB, patch)
2012-08-08 13:11 PDT, Eric Penner
no flags Details | Formatted Diff | Diff
rebase (24.29 KB, patch)
2012-08-08 13:48 PDT, Eric Penner
no flags Details | Formatted Diff | Diff
Patch (24.32 KB, patch)
2012-08-08 15:38 PDT, Eric Penner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Penner 2012-08-02 18:56:08 PDT
[chromium] Refactor loops to guarantee 'updated' flag correctness.
Comment 1 Eric Penner 2012-08-02 18:59:40 PDT
Created attachment 156236 [details]
Patch
Comment 2 Adrienne Walker 2012-08-03 09:48:29 PDT
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.
Comment 3 Eric Penner 2012-08-03 11:58:41 PDT
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.
Comment 4 Eric Penner 2012-08-06 13:42:16 PDT
Created attachment 156757 [details]
Patch
Comment 5 Eric Penner 2012-08-06 14:37:48 PDT
Created attachment 156761 [details]
rebase
Comment 6 WebKit Review Bot 2012-08-06 16:17:15 PDT
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
Comment 7 WebKit Review Bot 2012-08-06 16:17:19 PDT
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
Comment 8 Eric Penner 2012-08-06 17:44:15 PDT
Created attachment 156812 [details]
rebase_plus_extra_asserts
Comment 9 Eric Penner 2012-08-06 17:46:10 PDT
> 
> 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.
Comment 10 Dana Jansens 2012-08-07 11:39:28 PDT
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?)
Comment 11 Adrienne Walker 2012-08-07 15:47:21 PDT
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.
Comment 12 Eric Penner 2012-08-07 17:13:33 PDT
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.
Comment 13 Adrienne Walker 2012-08-07 17:22:07 PDT
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.
Comment 14 Eric Penner 2012-08-07 17:46:45 PDT
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.
Comment 15 Dana Jansens 2012-08-08 10:04:35 PDT
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.
Comment 16 Eric Penner 2012-08-08 13:11:18 PDT
Created attachment 157276 [details]
round_2
Comment 17 Eric Penner 2012-08-08 13:12:43 PDT
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.
Comment 18 Eric Penner 2012-08-08 13:48:48 PDT
Created attachment 157283 [details]
rebase
Comment 19 Adrienne Walker 2012-08-08 15:02:47 PDT
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.
Comment 20 Eric Penner 2012-08-08 15:11:31 PDT
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.
Comment 21 Eric Penner 2012-08-08 15:12:08 PDT
Comment on attachment 157283 [details]
rebase

Tested in chrome, gtg.
Comment 22 WebKit Review Bot 2012-08-08 15:20:46 PDT
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
Comment 23 Eric Penner 2012-08-08 15:34:31 PDT
Arg! sorry I removed the 'oops'. I gotta refine my process for making these logs.
Comment 24 Eric Penner 2012-08-08 15:38:43 PDT
Created attachment 157314 [details]
Patch
Comment 25 Eric Penner 2012-08-08 15:42:01 PDT
Comment on attachment 157314 [details]
Patch

Fixed log, gtg.
Comment 26 Adrienne Walker 2012-08-08 15:42:33 PDT
Comment on attachment 157314 [details]
Patch

R=me.  Let's try this again.
Comment 27 WebKit Review Bot 2012-08-08 16:50:51 PDT
Comment on attachment 157314 [details]
Patch

Clearing flags on attachment: 157314

Committed r125114: <http://trac.webkit.org/changeset/125114>
Comment 28 WebKit Review Bot 2012-08-08 16:50:57 PDT
All reviewed patches have been landed.  Closing bug.