Bug 133020

Summary: [wk2] RemoteLayerBackingStore front buffers should be purgeable when unparented
Product: WebKit Reporter: Tim Horton <thorton>
Component: WebKit2Assignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
preliminary
none
patch
simon.fraser: review-
patch
none
patch simon.fraser: review+

Description Tim Horton 2014-05-16 18:37:39 PDT
And also purgeable front buffers for unparented layers, etc.
Comment 1 Tim Horton 2014-05-16 18:51:39 PDT
Created attachment 231613 [details]
preliminary
Comment 2 Tim Horton 2014-05-20 00:29:21 PDT
<rdar://problem/16521736>
Comment 3 Tim Horton 2014-05-20 00:39:15 PDT
Created attachment 231755 [details]
patch
Comment 4 Simon Fraser (smfr) 2014-05-20 09:06:53 PDT
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.
Comment 5 Tim Horton 2014-05-20 09:23:47 PDT
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.
Comment 6 Tim Horton 2014-05-20 12:27:35 PDT
Created attachment 231783 [details]
patch
Comment 7 Tim Horton 2014-05-22 00:24:05 PDT
Created attachment 231865 [details]
patch
Comment 8 Tim Horton 2014-05-26 23:15:43 PDT
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