Bug 100169 – We should make TileCache tiles the size of the tile coverage rect when we can't do fast scrolling

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

Some websites that don't do fast scrolling still scroll slower than they do with tiled drawing disabled. https://bugs.webkit.org/show_bug.cgi?id=99768 addressed some of this performance problem, but there is still more ground to make up. I propose that we make tiles the size of the window when we can't do fast scrolling. This seems to have a noticeable improvement on sites like Facebook.

<rdar://problem/12505021>

[Beth Dakin]

Created attachment 170260 [details]
Patch

[Darin Adler]

Comment on attachment 170260 [details]
Patch

If the TileCache already has to know the default tile size for the adjustTileSizeForCoverageRect, then I don’t see any reason that WebTileCacheLayer should also have to pass it in. The TileCache should just know what it is.

[Beth Dakin]

(In reply to comment #2)
> (From update of attachment 170260 [details])
> If the TileCache already has to know the default tile size for the adjustTileSizeForCoverageRect, then I don’t see any reason that WebTileCacheLayer should also have to pass it in. The TileCache should just know what it is.

Good point. Will fix.

[Beth Dakin]

Created attachment 170267 [details]
Patch

[Darin Adler]

Comment on attachment 170267 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:49
> +static const int defaultTileCacheWidth = 512;
> +static const int defaultTileCacheHeight = 512;

Normally it would be good to include a comment stating why this is the default.

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:59
> +    , m_tileSize(IntSize(defaultTileCacheWidth, defaultTileCacheHeight))

I think you can leave out IntSize() here since it’s already the initializer list for an IntSize.

[Simon Fraser (smfr)]

Comment on attachment 170267 [details]
Patch

r- for lack of test.You can test this now (see test in plaform/mac/tiled-scrolling.

[Simon Fraser (smfr)]

Beth and I also noticed some bugs here when a page toggles between threaded and non-threaded scrolling, which I think also needs to be covered by tests.

[Beth Dakin]

Created attachment 170455 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 170455 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:59
> +    , m_tileSize(IntSize(defaultTileCacheWidth, defaultTileCacheHeight))

You shouldn't need the IntSize() there.

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:317
> +    // If the m_tileSize and clampedRect.size() are the same in either dimension, then we
> +    // don't want to overdraw, so explicity set x/y to 0 in that case.
> +    if (clampedRect.maxX() !=  m_tileSize.width())
> +        bottomRight.setX(max(clampedRect.maxX() / m_tileSize.width(), 0));
> +    else
> +        bottomRight.setX(0);
> +
> +    if (clampedRect.maxY() !=  m_tileSize.height())
> +        bottomRight.setY(max(clampedRect.maxY() / m_tileSize.height(), 0));
> +    else
> +        bottomRight.setY(0);

Can't this be done with ceil()? or floor(), since presumably the issue occurs at other integral boundaries too.

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:349
> +void TileCache::adjustTileSizeForCoverageRect(const IntRect& coverageRect)
> +{
> +    if (m_tileCoverage == CoverageForVisibleArea) {
> +        m_tileSize = coverageRect.size();
> +        return;
> +    }
> +
> +    m_tileSize = IntSize(defaultTileCacheWidth, defaultTileCacheHeight);
> +}

I think it would be clearer to make this  IntSize tileSizeForCoverageRect(const IntRect&) const. That way the changing of m_tileSize isn't hidden from the caller.

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:441
> +    int numTiles = 0;
>      for (int y = topLeft.y(); y <= bottomRight.y(); ++y) {
>          for (int x = topLeft.x(); x <= bottomRight.x(); ++x) {
> +            numTiles++;
>              TileIndex tileIndex(x, y);

You don't seem to be using numTiles.

[Beth Dakin]

(In reply to comment #9)
> (From update of attachment 170455 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170455&action=review
> 
> > Source/WebCore/platform/graphics/ca/mac/TileCache.mm:59
> > +    , m_tileSize(IntSize(defaultTileCacheWidth, defaultTileCacheHeight))
> 
> You shouldn't need the IntSize() there.
> 

Doh! I meant to remove that when Darin pointed it out. Thanks for the reminder.

> > Source/WebCore/platform/graphics/ca/mac/TileCache.mm:317
> > +    // If the m_tileSize and clampedRect.size() are the same in either dimension, then we
> > +    // don't want to overdraw, so explicity set x/y to 0 in that case.
> > +    if (clampedRect.maxX() !=  m_tileSize.width())
> > +        bottomRight.setX(max(clampedRect.maxX() / m_tileSize.width(), 0));
> > +    else
> > +        bottomRight.setX(0);
> > +
> > +    if (clampedRect.maxY() !=  m_tileSize.height())
> > +        bottomRight.setY(max(clampedRect.maxY() / m_tileSize.height(), 0));
> > +    else
> > +        bottomRight.setY(0);
> 
> Can't this be done with ceil()? or floor(), since presumably the issue occurs at other integral boundaries too.
> 

You are definitely right that this can happen at other integral boundaries, so I have a fix for that. I still can't figure out how to do this in one line though… I'll try a bit more.

> > Source/WebCore/platform/graphics/ca/mac/TileCache.mm:349
> > +void TileCache::adjustTileSizeForCoverageRect(const IntRect& coverageRect)
> > +{
> > +    if (m_tileCoverage == CoverageForVisibleArea) {
> > +        m_tileSize = coverageRect.size();
> > +        return;
> > +    }
> > +
> > +    m_tileSize = IntSize(defaultTileCacheWidth, defaultTileCacheHeight);
> > +}
> 
> I think it would be clearer to make this  IntSize tileSizeForCoverageRect(const IntRect&) const. That way the changing of m_tileSize isn't hidden from the caller.
> 

Will change.

> > Source/WebCore/platform/graphics/ca/mac/TileCache.mm:441
> > +    int numTiles = 0;
> >      for (int y = topLeft.y(); y <= bottomRight.y(); ++y) {
> >          for (int x = topLeft.x(); x <= bottomRight.x(); ++x) {
> > +            numTiles++;
> >              TileIndex tileIndex(x, y);
> 
> You don't seem to be using numTiles.

Oops, this was debugging code. Will remove.

[Beth Dakin]

Created attachment 170472 [details]
Patch

[Beth Dakin]

Thanks! Committed http://trac.webkit.org/changeset/132427

[Simon Fraser (smfr)]

This change affected tiled layer tile caches as well as the page tile cache, which was unintentional. Bug 100323.