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>
Created attachment 170260 [details] Patch
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.
(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.
Created attachment 170267 [details] Patch
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 on attachment 170267 [details] Patch r- for lack of test.You can test this now (see test in plaform/mac/tiled-scrolling.
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.
Created attachment 170455 [details] Patch
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.
(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.
Created attachment 170472 [details] Patch
Thanks! Committed http://trac.webkit.org/changeset/132427
This change affected tiled layer tile caches as well as the page tile cache, which was unintentional. Bug 100323.