Bug 149977 – Add debug settings for using giant tiles (4096x4096)

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

[Said Abou-Hallawa]

This setting will be used for debugging and evaluating the overhead which may be incurred due to a large tile size.

[Said Abou-Hallawa]

Created attachment 262808 [details]
Patch

[Tim Horton]

Comment on attachment 262808 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:307
> +std::unique_ptr<GraphicsLayer> GraphicsLayer::create(GraphicsLayerFactory* factory, GraphicsLayerClient& client, Type layerType, const IntSize& tileSize)

It's pretty weird that we're passing tileSize in GraphicsLayer::create for every layer when most layers aren't tiled.

Can't we just make a static class method on TiledBacking and change the defaults there? It would make this patch a lot smaller.

[Said Abou-Hallawa]

Created attachment 262816 [details]
Patch

[Said Abou-Hallawa]

Created attachment 262821 [details]
Patch

[Jeff Miller]

Comment on attachment 262821 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/WKPreferencesRefPrivate.h:317
> +WK_EXPORT void WKPreferencesSetSingleLargeTile(WKPreferencesRef, bool);

This name makes it seem like the function should be passed a single large tile. Can you change it to something like WKPreferencesSetUsesSingleLargeTile()?

This should probably be changed in the underlying WebPreferences implementation as well.

> Source/WebKit2/UIProcess/API/C/WKPreferencesRefPrivate.h:318
> +WK_EXPORT bool WKPreferencesGetSingleLargeTile(WKPreferencesRef);

Same comment here. WKPreferencesGetUsesSingleLargeTile()?

[Said Abou-Hallawa]

Created attachment 262875 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 262821 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/C/WKPreferencesRefPrivate.h:318
>> +WK_EXPORT bool WKPreferencesGetSingleLargeTile(WKPreferencesRef);
> 
> Same comment here. WKPreferencesGetUsesSingleLargeTile()?

Done, the name was changed.

[Simon Fraser (smfr)]

Comment on attachment 262875 [details]
Patch

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

> Source/WebCore/page/Settings.in:204
> +usesSingleLargeTile initial=false

"use single" is a misnomer. It may still create multiple tiles. I'd call it useGiantTiles or something.

> Source/WebCore/platform/graphics/TiledBacking.h:31
> +inline static IntSize defaultTileSize(bool usesSingleLargeTile = false)

usesSingleLargeTile -> useGiantTiles

> Source/WebCore/platform/graphics/TiledBacking.h:35
> +    // The width and height of a single tile in a tiled layer. Should be large enough to
> +    // avoid lots of small tiles (and therefore lots of drawing callbacks), but small enough
> +    // to keep the overall tile cost low.

I don't think this comment adds anything.

> Source/WebCore/platform/graphics/TiledBacking.h:38
> +    // This is an experimental value for Tdebugging and evaluating the overhead which may be

Tdebugging

[Tim Horton]

Comment on attachment 262875 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/PlatformCALayer.h:118
> +    void setTileSize(const IntSize& tileSize) { m_tileSize = tileSize; }

I still think we should avoid the getter/setter entirely. Add something to PlatformCALayerClient that returns the state of the *setting*  (the boolean state), and make the decision by calling that. Just a thought!

> Source/WebKit/mac/ChangeLog:1
> [phone]  Said Abou-Hallawa  <[e-mail]>

There is no need for WebKit1 SPI for this. Please omit it.

[Said Abou-Hallawa]

Created attachment 262938 [details]
Patch

[Said Abou-Hallawa]

Created attachment 262939 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 262875 [details]
Patch

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

>> Source/WebCore/page/Settings.in:204
>> +usesSingleLargeTile initial=false
> 
> "use single" is a misnomer. It may still create multiple tiles. I'd call it useGiantTiles or something.

Done. The name was changed to useGiantTiles.

>> Source/WebCore/platform/graphics/TiledBacking.h:31
>> +inline static IntSize defaultTileSize(bool usesSingleLargeTile = false)
> 
> usesSingleLargeTile -> useGiantTiles

Done. The argument was changed to useGiantTiles.

>> Source/WebCore/platform/graphics/TiledBacking.h:35
>> +    // to keep the overall tile cost low.
> 
> I don't think this comment adds anything.

Done. The comment was deleted.

>> Source/WebCore/platform/graphics/TiledBacking.h:38
>> +    // This is an experimental value for Tdebugging and evaluating the overhead which may be
> 
> Tdebugging

Done. The spelling mistake was fixed.

>> Source/WebCore/platform/graphics/ca/PlatformCALayer.h:118
>> +    void setTileSize(const IntSize& tileSize) { m_tileSize = tileSize; }
> 
> I still think we should avoid the getter/setter entirely. Add something to PlatformCALayerClient that returns the state of the *setting*  (the boolean state), and make the decision by calling that. Just a thought!

Done. Since  GraphicsLayerCA is derived from GraphicsLayer and PlatformCALayerClient, the setter is defined as the virtual function GraphicsLayer::setTileSize and the getter is defined as the virtual function PlatformCALayerClient::platformCALayerTileSize(). GraphicsLayerCA overrides both. The RenderLayerBacking::createGraphicsLayer() will call the setter. And TileController::tileSize() will call the getter.

>> Source/WebKit/mac/ChangeLog:1
>> [phone]  Said Abou-Hallawa  <[e-mail]>
> 
> There is no need for WebKit1 SPI for this. Please omit it.

Done. WebKit SPI was removed.

[Said Abou-Hallawa]

Created attachment 262951 [details]
Patch

[zalan]

Comment on attachment 262951 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsLayerClient.h:29
> +#include "FloatRect.h"

Do you really need the FloatRect.h?

> Source/WebCore/platform/graphics/TiledBacking.h:31
> +inline static IntSize defaultTileSize(bool useGiantTiles = false)

Could you use enum class instead? -this could also be a static TiledBacking::defaultTileSize() instead of this standalone function, but I don't really have a strong preference.

[Said Abou-Hallawa]

Created attachment 262989 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 262951 [details]
Patch

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

>> Source/WebCore/platform/graphics/GraphicsLayerClient.h:29
>> +#include "FloatRect.h"
> 
> Do you really need the FloatRect.h?

TiledBacking.h was missing forward declaration for FloatPoint and FloatRect. I added them in there and deleted the #include "FloatRect.h" from GraphicsLayerClient.h.

>> Source/WebCore/platform/graphics/TiledBacking.h:31
>> +inline static IntSize defaultTileSize(bool useGiantTiles = false)
> 
> Could you use enum class instead? -this could also be a static TiledBacking::defaultTileSize() instead of this standalone function, but I don't really have a strong preference.

An enum called TileSizeMode is added and used as the parameter to defaultTileSize() instead of passing the bool.

[WebKit Commit Bot]

Comment on attachment 262989 [details]
Patch

Rejecting attachment 262989 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 262989, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/279980

[Said Abou-Hallawa]

Created attachment 262991 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 262991 [details]
Patch

Clearing flags on attachment: 262991

Committed r190998: <http://trac.webkit.org/changeset/190998>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

<rdar://problem/23017093>