Currently the default tile dimensions and maximum un-tiled layer size are hardcoded in TiledLayerChromium. This makes it hard to experiment with different tiling strategies. Exposing these values via WebSettings will allow us to pass the desired tiling parameters via command-line flags.
Created attachment 142029 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 142029 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142029&action=review I guess we may have to think and experiment a bit with occlusion tracking and different tile sizes. The 160 seemed to work well with 256x256 tiles. If tiles are 100x100, then tracking 100x100 areas would probably yield some benefits (or maybe not!). Anyhow, something for us to consider. > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:118 > + const CCSettings settings = layerTreeHost()->settings(); reference?
Comment on attachment 142029 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142029&action=review > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:47 > +// When tiling is enabled, use tiles of this size. The LTH > +// settings can override these values. we shouldn't need these values at all since we always grab them off the CCSettings. Can you just delete them? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:116 > + ASSERT(layerTreeHost()); do we always have a valid layerTreeHost in tests? > Source/WebKit/chromium/public/WebSettings.h:151 > + virtual void setDefaultTileWidth(int) = 0; > + virtual void setDefaultTileHeight(int) = 0; > + virtual void setMaxUntiledLayerWidth(int) = 0; > + virtual void setMaxUntiledLayerHeight(int) = 0; rather than width/height pairs can the tile parameters be WebRects (and IntRects in the WebCore code)? > Source/WebKit/chromium/src/WebSettingsImpl.h:151 > + virtual void setDefaultTileWidth(int); > + virtual void setDefaultTileHeight(int); > + virtual void setMaxUntiledLayerWidth(int); > + virtual void setMaxUntiledLayerHeight(int); can you put these with the rest of the virtuals (above showFPSCounter/etc)?
(In reply to comment #4) > (From update of attachment 142029 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142029&action=review > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:47 > > +// When tiling is enabled, use tiles of this size. The LTH > > +// settings can override these values. > > we shouldn't need these values at all since we always grab them off the CCSettings. Can you just delete them? They are used in the constructor to create the initial CCLayerTilingData. In theory should be able to function even if updateTileSizeAndTilingOption() isn't called so we probably want to initialize it with something reasonable. > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:116 > > + ASSERT(layerTreeHost()); > > do we always have a valid layerTreeHost in tests? It's used a bit further down in this function without any checks so I assume yes. I just put in the assert just in case. > > > Source/WebKit/chromium/public/WebSettings.h:151 > > + virtual void setDefaultTileWidth(int) = 0; > > + virtual void setDefaultTileHeight(int) = 0; > > + virtual void setMaxUntiledLayerWidth(int) = 0; > > + virtual void setMaxUntiledLayerHeight(int) = 0; > > rather than width/height pairs can the tile parameters be WebRects (and IntRects in the WebCore code)? Will do. > > > Source/WebKit/chromium/src/WebSettingsImpl.h:151 > > + virtual void setDefaultTileWidth(int); > > + virtual void setDefaultTileHeight(int); > > + virtual void setMaxUntiledLayerWidth(int); > > + virtual void setMaxUntiledLayerHeight(int); > > can you put these with the rest of the virtuals (above showFPSCounter/etc)? Will do.
Created attachment 142492 [details] Patch
(In reply to comment #6) > Created an attachment (id=142492) [details] > Patch Addressed review comments. Please take another look!
Comment on attachment 142492 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142492&action=review > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:129 > + const bool anyDimensionLarge = contentBounds().width() > settings.maxUntiledLayerSize.width() || contentBounds().height() > settings.maxUntiledLayerSize.height(); > + const bool anyDimensionOneTile = (contentBounds().width() <= settings.defaultTileSize.width() || contentBounds().height() <= settings.defaultTileSize.height()) > + && (contentBounds().width() * contentBounds().height()) <= (settings.maxUntiledLayerSize.width() * settings.maxUntiledLayerSize.height()); these are all quite a mouthful. any way to simplify these (maybe store some repeated invocations like contentBounds().width() in locals)? the area check appears new, what's it for? > Source/WebKit/chromium/public/WebSettings.h:150 > + virtual void setDefaultTileSize(const WebSize&) = 0; > + virtual void setMaxUntiledLayerSize(const WebSize&) = 0; I think we should pass WebSize by value in the WK API, it's just a struct with 2 ints. const ref is overkill
(In reply to comment #8) > (From update of attachment 142492 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142492&action=review > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:129 > > + const bool anyDimensionLarge = contentBounds().width() > settings.maxUntiledLayerSize.width() || contentBounds().height() > settings.maxUntiledLayerSize.height(); > > + const bool anyDimensionOneTile = (contentBounds().width() <= settings.defaultTileSize.width() || contentBounds().height() <= settings.defaultTileSize.height()) > > + && (contentBounds().width() * contentBounds().height()) <= (settings.maxUntiledLayerSize.width() * settings.maxUntiledLayerSize.height()); > > these are all quite a mouthful. any way to simplify these (maybe store some repeated invocations like contentBounds().width() in locals)? > > the area check appears new, what's it for? The heuristic was originally in place to deal with skinny long layers such as scrollbars to avoid creating lots of tiles for them. However, if you allow for arbitrary tiling sizes, skinny becomes somewhat less well defined. Case in point: Imagine you set your defaultTileSize to (1024, 256) in order to make wide but short tiles that could be more optimal for vertical scrolling. If your window size drops to 1000 pixels wide then with the old logic you would end up creating a single tile of (1000, pageHeight) pixels up to pageHeight == maxTextureSize for the root layer. The resulting texture could end up being quite large. With this added logic, we only trigger the single-tile special case if the resulting overall tile area will be smaller than the area of the largest layer we're comfortable not tiling. We actually had a similar problem in: http://code.google.com/p/chromium/issues/detail?id=97241#c7 so I figured it's a good fix regardless. New patch addressing the rest of the feedback is on its way. Thanks. > > > Source/WebKit/chromium/public/WebSettings.h:150 > > + virtual void setDefaultTileSize(const WebSize&) = 0; > > + virtual void setMaxUntiledLayerSize(const WebSize&) = 0; > > I think we should pass WebSize by value in the WK API, it's just a struct with 2 ints. const ref is overkill
Created attachment 142522 [details] Patch
Comment on attachment 142522 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142522&action=review > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:49 > +// When tiling is enabled, use tiles of this size, unless the LTH > +// settings provide different values. > static int defaultTileWidth = 256; > static int defaultTileHeight = 256; It still seems a little weird to have these values here, as if they'll ever be used. I'd almost rather just pass IntSize() as the initial tile size so that there can be no mistake about where the tile size is coming from.
Created attachment 142587 [details] Patch
(In reply to comment #11) > (From update of attachment 142522 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142522&action=review > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:49 > > +// When tiling is enabled, use tiles of this size, unless the LTH > > +// settings provide different values. > > static int defaultTileWidth = 256; > > static int defaultTileHeight = 256; > > It still seems a little weird to have these values here, as if they'll ever be used. I'd almost rather just pass IntSize() as the initial tile size so that there can be no mistake about where the tile size is coming from. Makes sense. I did what you suggested in the latest patch!
Comment on attachment 142587 [details] Patch Thanks! R=me.
Comment on attachment 142587 [details] Patch Clearing flags on attachment: 142587 Committed r117521: <http://trac.webkit.org/changeset/117521>
All reviewed patches have been landed. Closing bug.