Bug 106061 – [wk2] Remove offscreen tiles from the layer tree

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

[Tim Horton]

We should remove TileCache tiles from the layer tree when they're not visible so that the OS can manage their memory as it wishes. The exact semantics haven't been worked out yet, but I have a preliminary patch.

<rdar://problem/12761821>

[Tim Horton]

Created attachment 181297 [details]
possibly a patch

[Build Bot]

Comment on attachment 181297 [details]
possibly a patch

Attachment 181297 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15677183

New failing tests:
compositing/tiling/tiled-layer-resize.html
compositing/tiling/rotated-tiled-preserve3d-clamped.html
fast/sub-pixel/sub-pixel-iframe-copy-on-scroll.html
fast/sub-pixel/sub-pixel-accumulates-to-layers.html
compositing/tiling/rotated-tiled-clamped.html
fast/sub-pixel/transformed-iframe-copy-on-scroll.html

[Simon Fraser (smfr)]

Comment on attachment 181297 [details]
possibly a patch

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

> Source/WebCore/page/Settings.cpp:189
> +    , m_aggressiveTileRetentionEnabled(false)

Why isn't m_unparentOffscreenTilesEnabled initialized?

> Source/WebCore/platform/graphics/TiledBacking.h:78
> +    virtual void setAggressiveTileRetentionEnabled(bool) = 0;
> +    virtual bool aggressiveTileRetentionEnabled() const = 0;
> +    
> +    virtual void setUnparentOffscreenTilesEnabled(bool) = 0;
> +    virtual bool unparentOffscreenTilesEnabled() const = 0;
> +    

These sound like setting plumbing. I think they would read better as
setAggressivelyRetainsTiles()
setUnparentsOffscreenTiles() etc.

> Source/WebCore/platform/graphics/ca/mac/TileCache.h:143
> +        RemoveTilesFromLayerTree = 1 << 2

I see no downside to always unparenting non TCR tiles. Maybe we should just always do this?

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:320
> +    if (m_isInWindow) {
> +        scheduleTileRevalidation(0);
> +    } else {
>          const double tileRevalidationTimeout = 4;
>          scheduleTileRevalidation(tileRevalidationTimeout);
>      }

This would be better as:
scheduleTileRevalidation(m_isInWindow ? 0 : tileRevalidationTimeout);

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:452
> +    if (!m_unparentOffscreenTilesEnabled || m_isInWindow)
> +        revalidationPolicyFlags = CreatePrimaryTiles | PruneSecondaryTiles;
> +    else
> +        revalidationPolicyFlags = PruneSecondaryTiles | RemoveTilesFromLayerTree;

This gets simpler if we always remove tiles. CreatePrimaryTiles is only to avoid making primary tiles in background tabs, right?

[Tim Horton]

Created attachment 181357 [details]
better patch

I'm going to look at the test failures now.

[Tim Horton]

(In reply to comment #2)
> (From update of attachment 181297 [details])
> Attachment 181297 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/15677183
> 
> New failing tests:
> compositing/tiling/tiled-layer-resize.html
> compositing/tiling/rotated-tiled-preserve3d-clamped.html
> fast/sub-pixel/sub-pixel-iframe-copy-on-scroll.html
> fast/sub-pixel/sub-pixel-accumulates-to-layers.html
> compositing/tiling/rotated-tiled-clamped.html
> fast/sub-pixel/transformed-iframe-copy-on-scroll.html

Three I can't reproduce (compositing/tiling), three are "missing expected result" :\ (fast/sub-pixel).

[Simon Fraser (smfr)]

Comment on attachment 181357 [details]
better patch

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

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:594
> +                    if (m_unparentsOffscreenTiles)

Why isn't this checking for the RemoveTilesFromLayerTree flag?

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:653
> +    if (validationPolicy & RemoveTilesFromLayerTree) {

The confusion  between m_unparentsOffscreenTiles and RemoveTilesFromLayerTree is great!

[Tim Horton]

Created attachment 181386 [details]
simpler patch, always unparent (remove the setting)

[Tim Horton]

Thanks, Simon!

http://trac.webkit.org/changeset/138858

[Alexey Proskuryakov]

This caused bug 106205.

[Tim Horton]

This also caused https://bugs.webkit.org/show_bug.cgi?id=108864.