Bug 132786 – Triple-buffer RemoteLayerBackingStore

• Component: WebKit2
• Status: RESOLVED FIXED

[Tim Horton]

From one... to two... to three.

We need three buffers because we're currently unable to synchronize with the render server to swap when they're not in use, so we end up throwing surfaces away and wasting expensive work (and deleting them is not free, either!). The pool helps, but not enough (the pool is much more useful for e.g. layers that come and go, like tiles while scrolling).

We'll try to keep them purgeable as much as possible so that the memory hit is not significant (but, this should also match CA when painting repeatedly).

This is a three-orders-of-magnitude reduction in time spent in IOSurface::create on http://www.kevs3d.co.uk/dev/canvasmark/...

[Radar WebKit Bug Importer]

<rdar://problem/16877498>

[Tim Horton]

Created attachment 231275 [details]
patch

[Tim Horton]

Need to build mac and sim to make sure I haven't broken anything.

[WebKit Commit Bot]

Attachment 231275 [details] did not pass style-queue:


ERROR: Source/WebKit2/Shared/mac/RemoteLayerBackingStore.h:92:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 231275 [details]
patch

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

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:403
> +    if (type == BufferType::Front && m_frontBuffer.surface && m_frontBuffer.isVolatile != isVolatile) {
> +        if (!isVolatile || !m_frontBuffer.surface->isInUse()) {
> +            IOSurface::SurfaceState previousState = m_frontBuffer.surface->setIsVolatile(isVolatile);
> +            m_frontBuffer.isVolatile = isVolatile;
> +
> +            // Becoming non-volatile and the front buffer was purged, so we need to repaint.
> +            if (!isVolatile && (previousState == IOSurface::SurfaceState::Empty))
> +                setNeedsDisplay();
> +        } else
> +            return false;
> +    } else if (type == BufferType::Back && m_backBuffer.surface && m_backBuffer.isVolatile != isVolatile) {
> +        if (!isVolatile || !m_backBuffer.surface->isInUse()) {
> +            m_backBuffer.surface->setIsVolatile(isVolatile);
> +            m_backBuffer.isVolatile = isVolatile;
> +        } else
> +            return false;
> +    } else if (type == BufferType::SecondaryBack && m_secondaryBackBuffer.surface && m_secondaryBackBuffer.isVolatile != isVolatile) {
> +        if (!isVolatile || !m_secondaryBackBuffer.surface->isInUse()) {
> +            m_secondaryBackBuffer.surface->setIsVolatile(isVolatile);
> +            m_secondaryBackBuffer.isVolatile = isVolatile;
> +        } else
> +            return false;

This logic is pretty confusing. Can you convert this to a switch(type)?

[Tim Horton]

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