Bug 131213 - [iOS WebKit2] Mark back buffers purgeable when they're not being painted frequently
Summary: [iOS WebKit2] Mark back buffers purgeable when they're not being painted freq...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-04-03 23:23 PDT by Tim Horton
Modified: 2014-04-07 14:24 PDT (History)
4 users (show)

See Also:


Attachments
patch (38.23 KB, patch)
2014-04-04 01:27 PDT, Tim Horton
simon.fraser: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (575.41 KB, application/zip)
2014-04-04 02:28 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2014-04-03 23:23:12 PDT
<rdar://problem/15373906>
Comment 1 Tim Horton 2014-04-04 01:27:13 PDT
Created attachment 228581 [details]
patch
Comment 2 Build Bot 2014-04-04 02:28:13 PDT
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
Comment 3 Build Bot 2014-04-04 02:28:16 PDT
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
Comment 4 Simon Fraser (smfr) 2014-04-06 22:10:30 PDT
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?
Comment 5 Tim Horton 2014-04-07 14:24:07 PDT
http://trac.webkit.org/changeset/166886