Bug 77564 – Tile cache doesn't have an upper limit

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Anders Carlsson]

Tile cache doesn't have an upper limit

[Anders Carlsson]

Created attachment 124976 [details]
Patch

[Darin Adler]

Comment on attachment 124976 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:941
> +    // Let the client know that it needs to sync the layer state. We use this to ensure that
> +    // the layout is up to date before the individual tiles are painted.

The second half of the comment is the good part; the first sort of repeats what the code says and that is not needed. Merging the two sentences might make things clearer and shorter.

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:69
> +        // Revalidate the tiles right away if we don't have any. This avoids
> +        // flashing when transitioning from one page to another.

Same thought. The first sentence just says what the code does. A shorter comment that focused just on the “why” may be possible. Like:

    // We must revalidate immediately instead of using a timer when there are no tiles to avoid a flash when transitioning from one page to another.

Wording it that way makes it clear there is something else to say. Like explaining how “no tiles” happens in a page transition.

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:262
> +    // FIXME: Figure out the optimal values here.
> +    tileCoverageRect.inflateX(tileCoverageRect.width() / 2);
> +    tileCoverageRect.inflateY(tileCoverageRect.height() * 2);

Another comment should also indicate why you think these are good starting values. Such as “it’s common to have tall pages and scroll vertically so we keep more tiles above and below the current area than on the left and right”. Or whatever that current thinking is.

This code seems to do 5X in the vertical dimension and 2X in the horizontal. Or a border of 2X on top and bottom and 0.5X on left and right. Your comment in change log cites different figures.

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:274
> +            [tileLayer removeFromSuperlayer];
> +            [tileLayer setTileCache:0];

Can tileLayer be deallocated here as a side effect of the call to removeFromSuperlayer?

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:280
> +    // FIXME: Be more clever about which tiles to remove.

Unclear what more clever means. Maybe you could write a better comment?

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:298
> +            if (!result.second) {
> +                // We already have a layer for this tile.
> +                continue;
>              }

Somethings I think that if (result.first->second) is clearer than if (!result.second). I like how it’s closely related to the createTileLayer line of code. Obviously neither is all that clear until we change the return types so they use named data members instead of pair/first,second naming.

[Anders Carlsson]

(In reply to comment #2)
> 
> > Source/WebCore/platform/graphics/ca/mac/TileCache.mm:262
> > +    // FIXME: Figure out the optimal values here.
> > +    tileCoverageRect.inflateX(tileCoverageRect.width() / 2);
> > +    tileCoverageRect.inflateY(tileCoverageRect.height() * 2);
> 
> 
> This code seems to do 5X in the vertical dimension and 2X in the horizontal. Or a border of 2X on top and bottom and 0.5X on left and right. Your comment in change log cites different figures.

Changed the comments/code to be 2x horizontally and 3x vertically and added a better comment. I've also filed <rdar://problem/10790464> to track improving this.

> > Source/WebCore/platform/graphics/ca/mac/TileCache.mm:274
> > +            [tileLayer removeFromSuperlayer];
> > +            [tileLayer setTileCache:0];
> 
> Can tileLayer be deallocated here as a side effect of the call to removeFromSuperlayer?

No - it's still retained by the tile map.

> 
> > Source/WebCore/platform/graphics/ca/mac/TileCache.mm:280
> > +    // FIXME: Be more clever about which tiles to remove.
> 
> Unclear what more clever means. Maybe you could write a better comment?
> 

Done.

> > Source/WebCore/platform/graphics/ca/mac/TileCache.mm:298
> > +            if (!result.second) {
> > +                // We already have a layer for this tile.
> > +                continue;
> >              }
> 
> Somethings I think that if (result.first->second) is clearer than if (!result.second). I like how it’s closely related to the createTileLayer line of code. Obviously neither is all that clear until we change the return types so they use named data members instead of pair/first,second naming.

Fixed.

Thanks for the review!

[Anders Carlsson]

Committed r106482: <http://trac.webkit.org/changeset/106482>