Bug 131213 – [iOS WebKit2] Mark back buffers purgeable when they're not being painted frequently

• Component: WebKit2
• Status: RESOLVED FIXED

[Tim Horton]

<rdar://problem/15373906>

[Tim Horton]

Created attachment 228581 [details]
patch

[Build Bot]

Comment on attachment 228581 [details]
patch

Attachment 228581 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4771076601020416

New failing tests:
platform/mac/fast/scrolling/scroll-iframe-latched-mainframe.html
platform/mac/fast/scrolling/scroll-select-latched-mainframe.html
media/W3C/audio/canPlayType/canPlayType_application_octet_stream_with_codecs_1.html
platform/mac/fast/scrolling/scroll-div-latched-mainframe.html

[Build Bot]

Created attachment 228583 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Simon Fraser (smfr)]

Comment on attachment 228581 [details]
patch

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

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.h:126
> +    std::chrono::steady_clock::time_point m_lastDisplayTime;

std::so::verbose

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:373
> +    if (markVolatile)
> +        surface->setIsVolatile(true);
> +    else if (surface->setIsVolatile(false) == IOSurface::SurfaceState::Empty)
> +        return false;

if (markVolatile) {
  surface...
  return true;
}
...

Or do the "return false" test first.

Also the two booleans (markVolatile and the argument to setIsVolatile) make this code very confusing.

How about:

bool result = surface->setIsVolatile(markVolatile);
if (!markVolatile && !result)
  return false;

It would be clearer if you used an enum markVolatile/markNonVolatile.

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:402
> +        if (!applyVolatilityToSurface(m_frontSurface.get(), false))
> +            setNeedsDisplay();
> +        applyVolatilityToSurface(m_backSurface.get(), false);
> +        break;
> +    case Volatility::BackBufferVolatile:
> +        if (m_backSurface && m_backSurface->isInUse())
> +            return false;
> +        if (!applyVolatilityToSurface(m_frontSurface.get(), false))
> +            setNeedsDisplay();
> +        applyVolatilityToSurface(m_backSurface.get(), true);
> +        break;
> +    case Volatility::AllBuffersVolatile:
> +        if (m_frontSurface && m_frontSurface->isInUse())
> +            return false;
> +        if (m_backSurface && m_backSurface->isInUse())
> +            return false;
> +        applyVolatilityToSurface(m_frontSurface.get(), true);
> +        applyVolatilityToSurface(m_backSurface.get(), true);

false, false, false, true, true!

> Source/WebKit2/Shared/mac/RemoteLayerBackingStoreCollection.h:59
> +private:
> +    HashSet<RemoteLayerBackingStore*> m_liveBackingStore;
> +
> +    void markInactiveBackingStorePurgeable();
> +
> +    RemoteLayerTreeContext* m_context;
> +
> +    WebCore::Timer<RemoteLayerBackingStoreCollection> m_purgeabilityTimer;

Would prefer the member variables be grouped together.

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeContext.h:82
> +    std::unique_ptr<RemoteLayerBackingStoreCollection> m_backingStoreCollection;

Why not just have it by value?

[Tim Horton]

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