Bug 100169 - We should make TileCache tiles the size of the tile coverage rect when we can't do fast scrolling
Summary: We should make TileCache tiles the size of the tile coverage rect when we can...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-10-23 15:57 PDT by Beth Dakin
Modified: 2012-10-24 20:39 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.91 KB, patch)
2012-10-23 16:04 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (6.03 KB, patch)
2012-10-23 16:32 PDT, Beth Dakin
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (10.50 KB, patch)
2012-10-24 13:05 PDT, Beth Dakin
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (9.87 KB, patch)
2012-10-24 14:27 PDT, Beth Dakin
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2012-10-23 15:57:26 PDT
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>
Comment 1 Beth Dakin 2012-10-23 16:04:24 PDT
Created attachment 170260 [details]
Patch
Comment 2 Darin Adler 2012-10-23 16:09:24 PDT
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.
Comment 3 Beth Dakin 2012-10-23 16:11:44 PDT
(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.
Comment 4 Beth Dakin 2012-10-23 16:32:16 PDT
Created attachment 170267 [details]
Patch
Comment 5 Darin Adler 2012-10-23 16:45:48 PDT
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.
Comment 6 Simon Fraser (smfr) 2012-10-23 16:53:41 PDT
Comment on attachment 170267 [details]
Patch

r- for lack of test.You can test this now (see test in plaform/mac/tiled-scrolling.
Comment 7 Simon Fraser (smfr) 2012-10-23 17:57:17 PDT
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.
Comment 8 Beth Dakin 2012-10-24 13:05:16 PDT
Created attachment 170455 [details]
Patch
Comment 9 Simon Fraser (smfr) 2012-10-24 13:11:18 PDT
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.
Comment 10 Beth Dakin 2012-10-24 13:49:04 PDT
(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.
Comment 11 Beth Dakin 2012-10-24 14:27:07 PDT
Created attachment 170472 [details]
Patch
Comment 12 Beth Dakin 2012-10-24 17:21:00 PDT
Thanks! Committed http://trac.webkit.org/changeset/132427
Comment 13 Simon Fraser (smfr) 2012-10-24 20:39:41 PDT
This change affected tiled layer tile caches as well as the page tile cache, which was unintentional. Bug 100323.