Bug 133020 – [wk2] RemoteLayerBackingStore front buffers should be purgeable when unparented

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Tim Horton]

And also purgeable front buffers for unparented layers, etc.

[Tim Horton]

Created attachment 231613 [details]
preliminary

[Tim Horton]

<rdar://problem/16521736>

[Tim Horton]

Created attachment 231755 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 231755 [details]
patch

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

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:389
> +            if (isVolatile)
> +                m_frontBuffer.surface->clearGraphicsContext();

I was confused briefly by the "clearGraphicsContext" terminology, interpreting it as "paint the clear color into the graphics context", but it's really removeGraphicsContext() right?

> Source/WebKit2/Shared/mac/RemoteLayerBackingStoreCollection.mm:61
> +    if (!m_unparentedBackingStore.contains(backingStore))
> +        return;
> +    m_liveBackingStore.add(backingStore);
> +    m_unparentedBackingStore.remove(backingStore);

Can't this be done with just one hash lookup in m_unparentedBackingStore.contains?

> Source/WebKit2/Shared/mac/RemoteLayerBackingStoreCollection.mm:72
> +    if (!backingStore.setBufferVolatility(RemoteLayerBackingStore::BufferType::SecondaryBack, true))
> +        successfullyMadeBackingStoreVolatile = false;
> +    if (!backingStore.setBufferVolatility(RemoteLayerBackingStore::BufferType::Back, true))
> +        successfullyMadeBackingStoreVolatile = false;
> +    if (!backingStore.layer()->superlayer()) {

Would prefer blank lines between these statements.

> Source/WebKit2/Shared/mac/RemoteLayerBackingStoreCollection.mm:100
> +    if (m_unparentedBackingStore.contains(backingStore))
> +        return;
> +    m_unparentedBackingStore.add(backingStore);

Same.

> Source/WebKit2/Shared/mac/RemoteLayerBackingStoreCollection.mm:118
> +        successfullyMadeBackingStoreVolatile |= markBackingStoreVolatile(*backingStore, now);

Don't you want &= ?  successfullyMadeBackingStoreVolatile starts off true so |= won't do anything.

> Source/WebKit2/Shared/mac/RemoteLayerBackingStoreCollection.mm:121
> +        successfullyMadeBackingStoreVolatile |= markBackingStoreVolatileImmediately(*backingStore);

Same.

> Source/WebKit2/UIProcess/mac/RemoteLayerTreeHost.mm:106
> +    // Drop the contents of any layers which were unparented; the Web process will re-send
> +    // the backing store in the commit that reparents them.
> +    for (auto& layerOrView : m_layers.values()) {
> +        CALayer *layer = asLayer(layerOrView.get());
> +        if (layer.superlayer)
> +            continue;
> +        layer.contents = nullptr;
> +    }

How should we handle disconnected subtrees? Or do we only care about this for tiles? Seems odd to consider all layers here when it's really just about tiles.

[Tim Horton]

Comment on attachment 231755 [details]
patch

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

>> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:389
>> +                m_frontBuffer.surface->clearGraphicsContext();
> 
> I was confused briefly by the "clearGraphicsContext" terminology, interpreting it as "paint the clear color into the graphics context", but it's really removeGraphicsContext() right?

Indeed it is.

>> Source/WebKit2/Shared/mac/RemoteLayerBackingStoreCollection.mm:61
>> +    m_unparentedBackingStore.remove(backingStore);
> 
> Can't this be done with just one hash lookup in m_unparentedBackingStore.contains?

Sure!

>> Source/WebKit2/Shared/mac/RemoteLayerBackingStoreCollection.mm:118
>> +        successfullyMadeBackingStoreVolatile |= markBackingStoreVolatile(*backingStore, now);
> 
> Don't you want &= ?  successfullyMadeBackingStoreVolatile starts off true so |= won't do anything.

Indeed, good call. It would be nice if there were a way to assert that this was working correctly, but I don't think it's possible.

>> Source/WebKit2/UIProcess/mac/RemoteLayerTreeHost.mm:106
>> +    }
> 
> How should we handle disconnected subtrees? Or do we only care about this for tiles? Seems odd to consider all layers here when it's really just about tiles.

Why would this just be about tiles? This code (the purgeable front buffers of unparented layers bit) ought to work for all layers.

[Tim Horton]

Created attachment 231783 [details]
patch

[Tim Horton]

Created attachment 231865 [details]
patch

[Tim Horton]

I landed just the purgeable front buffers bit without swapping aggressive tile retention on, because I have some concerns which will be addressed in another bug, but multiple patches are blocked on purgeable front buffers.

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