Changeset 233268 in webkit


Ignore:
Timestamp:
Jun 27, 2018 11:22:06 AM (6 years ago)
Author:
Simon Fraser
Message:

​https://hackernoon.com/ uses lots of layer backing store
​https://bugs.webkit.org/show_bug.cgi?id=186909
rdar://problem/40257540

Reviewed by Tim Horton.

Source/bmalloc:

Drive-by typo fix.

  • bmalloc/Scavenger.cpp:

(bmalloc::dumpStats):

Source/WebCore:

The existing "backing store detached" logic, which was used to eliminate backing store
for compositing layers outside the viewport, had a number of bugs that allowed layers
to have backing store when they should not.

Specifically, any code path that ended up in setNeedsDisplay{InRect}() in PlatformCALayer
could trigger backing store creation on layers that should have never had any.

Rather than monkeypatch all the GraphicsLayerCA call sites that call setNeedsDisplay{InRect}(),
just bail early from the PlatformCALayer* methods that trigger repaints.

Tests didn't catch this because they just dumped the state of the backingStoreAttached flag. To fix this,
create backingStoreAttachedForTesting() which also tests whether the layer has contents.

Test: compositing/backing/backing-store-attachment-outside-viewport.html

  • platform/graphics/GraphicsLayer.cpp:

(WebCore::GraphicsLayer::dumpProperties const):
(showGraphicsLayerTree):

  • platform/graphics/GraphicsLayer.h:

(WebCore::GraphicsLayer::backingStoreAttachedForTesting const):

  • platform/graphics/GraphicsLayerClient.h:
  • platform/graphics/ca/GraphicsLayerCA.cpp:

(WebCore::GraphicsLayerCA::backingStoreAttachedForTesting const):
(WebCore::GraphicsLayerCA::setNeedsDisplay):

  • platform/graphics/ca/GraphicsLayerCA.h:
  • platform/graphics/ca/PlatformCALayer.h:
  • platform/graphics/ca/cocoa/PlatformCALayerCocoa.h:
  • platform/graphics/ca/cocoa/PlatformCALayerCocoa.mm:

(PlatformCALayerCocoa::setNeedsDisplay):
(PlatformCALayerCocoa::setNeedsDisplayInRect):
(PlatformCALayerCocoa::hasContents const):

Source/WebKit:

PlatformCALayerRemote was actually holding onto backing stores for layers with
backing store detached, which could increase memory use. When told that backing stores
are not attached, explicitly throw away the backing, and re-create it (via setNeedsDisplay)
when attached. This is now similar to what PlatformLayerCACocoa does.

  • WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.cpp:

(WebKit::PlatformCALayerRemote::setNeedsDisplayInRect):
(WebKit::PlatformCALayerRemote::setNeedsDisplay):
(WebKit::PlatformCALayerRemote::hasContents const):

  • WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.h:

LayoutTests:

New test.

  • compositing/backing/backing-store-attachment-outside-viewport-expected.txt: Added.
  • compositing/backing/backing-store-attachment-outside-viewport.html: Added.
Location:
trunk
Files:
3 added
18 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r233257 r233268  
     12018-06-27  Simon Fraser  <simon.fraser@apple.com>
     2
     3        https://hackernoon.com/ uses lots of layer backing store
     4        https://bugs.webkit.org/show_bug.cgi?id=186909
     5        rdar://problem/40257540
     6
     7        Reviewed by Tim Horton.
     8       
     9        New test.
     10
     11        * compositing/backing/backing-store-attachment-outside-viewport-expected.txt: Added.
     12        * compositing/backing/backing-store-attachment-outside-viewport.html: Added.
     13
    1142018-06-27  Nan Wang  <n_wang@apple.com>
    215

  • trunk/LayoutTests/fast/images/animated-gif-iframe-webkit-transform.html

    r179787 r233268  
    77<p id="description"></p>
    88<div id="scroller" style="width: 800px; overflow: hidden">
    9     <div id="scroller-cont" style="height: 245px; width: 1600px; position: relative; top: 0; left: 0; -webkit-transform: translate(0px, 0px) translateZ(0px);">
    10         <div id="wrapper1" style="-webkit-transform: translate3d(0px, 0px, 0px); width: 800px; height: 245px; float: left; margin: 0; padding: 0">
     9    <div id="scroller-cont" style="height: 245px; width: 1410px; position: relative; top: 0; left: 0; -webkit-transform: translate(0px, 0px) translateZ(0px);">
     10        <div id="wrapper1" style="-webkit-transform: translate3d(0px, 0px, 0px); width: 710px; height: 245px; float: left; margin: 0; padding: 0">
    1111            <iframe id="testFrame1" src="resources/iframe-with-animated-gif.html" style="width: 245px; height: 245px;" onload="frameLoaded()"></iframe>
    1212        </div>
    13         <div id="wrapper2" style="-webkit-transform: translate3d(0px, 0px, 0px); width: 800px; height: 245px; float: left; margin: 0; padding: 0">
    14             <iframe id="testFrame2" src="resources/iframe-with-animated-gif2.html" style="width: 245px; height: 245px;" onload="frameLoaded()"></iframe>
     13        <div id="wrapper2" style="-webkit-transform: translate3d(0px, 0px, 0px); width: 700px; height: 245px; float: left; margin: 0; padding: 0">
     14            <iframe id="testFrame2" src="resources/iframe-with-animated-gif2.html" style="margin-left: 100px; width: 245px; height: 245px;" onload="frameLoaded()"></iframe>
    1515        </div>
    1616    </div>
    … …  
    5353  debug("Translating images left so that first image is no longer visible, but second image is.");
    5454  forceLayout();
    55   document.getElementById("scroller-cont").style["-webkit-transform"] = "translate(-800px, 0px)";
     55  document.getElementById("scroller-cont").style["-webkit-transform"] = "translate(-610px, 0px)";
    5656  shouldBecomeEqual("isFirstImagePaused()", "true", checkSecondImageUnpaused);
    5757}

  • trunk/LayoutTests/fast/images/animated-gif-webkit-transform.html

    r217950 r233268  
    55</head>
    66<body onload="runTest()">
    7 <div id="scroller" style="width: 800px; overflow: hidden">
    8     <div id="scroller-cont" style="height: 245px; width: 1600px; position: relative; top: 0; left: 0; -webkit-transform: translate(0px, 0px) translateZ(0px);">
    9         <div id="wrapper1" style="-webkit-transform: translate3d(0px, 0px, 0px); width: 800px; height: 245px; float: left; margin: 0; padding: 0">
     7<div id="scroller" style="width: 800px; overflow: scroll">
     8    <div id="scroller-cont" style="height: 245px; width: 1410px; position: relative; top: 0; left: 0; -webkit-transform: translate(0px, 0px) translateZ(0px);">
     9        <div id="wrapper1" style="-webkit-transform: translate3d(0px, 0px, 0px); width: 710px; height: 245px; float: left; margin: 0; padding: 0">
    1010            <img id="a" src="resources/animated.gif"/>
    1111        </div>
    12         <div id="wrapper2" style="-webkit-transform: translate3d(0px, 0px, 0px); width: 800px; height: 245px; float: left; margin: 0; padding: 0">
    13             <img id="b" src="resources/animated-10color.gif"/>
     12        <div id="wrapper2" style="-webkit-transform: translate3d(0px, 0px, 0px); width: 700px; height: 245px; float: left; margin: 0; padding: 0">
     13            <img id="b" src="resources/animated-10color.gif" style="margin-left: 100px;"/>
    1414        </div>
    1515    </div>
    … …  
    4343  debug("Translating images left so that first image is no longer visible, but second image is.");
    4444  forceLayout();
    45   document.getElementById("scroller-cont").style["-webkit-transform"] = "translate(-800px, 0px)";
     45  document.getElementById("scroller-cont").style["-webkit-transform"] = "translate(-610px, 0px)";
    4646  shouldBecomeEqual("isFirstImagePaused()", "true", checkSecondImageUnpaused);
    4747}

  • trunk/Source/WebCore/ChangeLog

    r233267 r233268  
     12018-06-27  Simon Fraser  <simon.fraser@apple.com>
     2
     3        https://hackernoon.com/ uses lots of layer backing store
     4        https://bugs.webkit.org/show_bug.cgi?id=186909
     5        rdar://problem/40257540
     6
     7        Reviewed by Tim Horton.
     8       
     9        The existing "backing store detached" logic, which was used to eliminate backing store
     10        for compositing layers outside the viewport, had a number of bugs that allowed layers
     11        to have backing store when they should not.
     12       
     13        Specifically, any code path that ended up in setNeedsDisplay{InRect}() in PlatformCALayer
     14        could trigger backing store creation on layers that should have never had any.
     15       
     16        Rather than monkeypatch all the GraphicsLayerCA call sites that call setNeedsDisplay{InRect}(),
     17        just bail early from the PlatformCALayer* methods that trigger repaints.
     18       
     19        Tests didn't catch this because they just dumped the state of the backingStoreAttached flag. To fix this,
     20        create backingStoreAttachedForTesting() which also tests whether the layer has contents.
     21
     22        Test: compositing/backing/backing-store-attachment-outside-viewport.html
     23
     24        * platform/graphics/GraphicsLayer.cpp:
     25        (WebCore::GraphicsLayer::dumpProperties const):
     26        (showGraphicsLayerTree):
     27        * platform/graphics/GraphicsLayer.h:
     28        (WebCore::GraphicsLayer::backingStoreAttachedForTesting const):
     29        * platform/graphics/GraphicsLayerClient.h:
     30        * platform/graphics/ca/GraphicsLayerCA.cpp:
     31        (WebCore::GraphicsLayerCA::backingStoreAttachedForTesting const):
     32        (WebCore::GraphicsLayerCA::setNeedsDisplay):
     33        * platform/graphics/ca/GraphicsLayerCA.h:
     34        * platform/graphics/ca/PlatformCALayer.h:
     35        * platform/graphics/ca/cocoa/PlatformCALayerCocoa.h:
     36        * platform/graphics/ca/cocoa/PlatformCALayerCocoa.mm:
     37        (PlatformCALayerCocoa::setNeedsDisplay):
     38        (PlatformCALayerCocoa::setNeedsDisplayInRect):
     39        (PlatformCALayerCocoa::hasContents const):
     40
    1412018-06-27  David Kilzer  <ddkilzer@apple.com>
    242

  • trunk/Source/WebCore/platform/graphics/BitmapImage.cpp

    r232802 r233268  
    391391BitmapImage::StartAnimationStatus BitmapImage::internalStartAnimation()
    392392{
     393    LOG_WITH_STREAM(Images, stream << "BitmapImage " << this << " internalStartAnimation");
     394
    393395    if (!canAnimate())
    394396        return StartAnimationStatus::CannotStart;

  • trunk/Source/WebCore/platform/graphics/GraphicsLayer.cpp

    r232246 r233268  
    788788
    789789    if (behavior & LayerTreeAsTextIncludeBackingStoreAttached)
    790         ts << indent << "(backingStoreAttached " << backingStoreAttached() << ")\n";
     790        ts << indent << "(backingStoreAttached " << backingStoreAttachedForTesting() << ")\n";
    791791
    792792    if (!m_transform.isIdentity()) {
    … …  
    932932        return;
    933933
    934     String output = layer->layerTreeAsText(WebCore::LayerTreeAsTextDebug | WebCore::LayerTreeAsTextIncludeVisibleRects | WebCore::LayerTreeAsTextIncludeTileCaches | WebCore::LayerTreeAsTextIncludeContentLayers);
     934    String output = layer->layerTreeAsText(WebCore::LayerTreeAsTextShowAll);
    935935    fprintf(stderr, "%s\n", output.utf8().data());
    936936}

  • trunk/Source/WebCore/platform/graphics/GraphicsLayer.h

    r233189 r233268  
    553553
    554554    virtual bool backingStoreAttached() const { return true; }
     555    virtual bool backingStoreAttachedForTesting() const { return backingStoreAttached(); }
    555556
    556557    void setCanDetachBackingStore(bool b) { m_canDetachBackingStore = b; }

  • trunk/Source/WebCore/platform/graphics/GraphicsLayerClient.h

    r229174 r233268  
    7373    LayerTreeAsTextIncludeAcceleratesDrawing    = 1 << 7,
    7474    LayerTreeAsTextIncludeBackingStoreAttached  = 1 << 8,
     75    LayerTreeAsTextShowAll                      = 0xFFFF
    7576};
    7677typedef unsigned LayerTreeAsTextBehavior;

  • trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp

    r233122 r233268  
    882882}
    883883
     884bool GraphicsLayerCA::backingStoreAttachedForTesting() const
     885{
     886    return m_layer->backingStoreAttached() || m_layer->hasContents();
     887}
     888
    884889void GraphicsLayerCA::setNeedsDisplay()
    885890{
    886891    if (!drawsContent())
    887         return;
    888 
    889     if (!backingStoreAttached())
    890892        return;
    891893
    … …  
    23492351            || commitState.ancestorWithTransformAnimationIntersectsCoverageRect // FIXME: Compute backing exactly for descendants of animating layers.
    23502352            || (isRunningTransformAnimation() && !animationExtent()); // Create backing if we don't know the animation extent.
     2353
     2354        LOG_WITH_STREAM(Compositing, stream << "GraphicsLayerCA " << this << " id " << primaryLayerID() << " setBackingStoreAttached: " << requiresBacking);
    23512355
    23522356        m_layer->setBackingStoreAttached(requiresBacking);

  • trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h

    r231840 r233268  
    265265
    266266    WEBCORE_EXPORT bool backingStoreAttached() const override;
     267    WEBCORE_EXPORT bool backingStoreAttachedForTesting() const override;
    267268
    268269    bool animationIsRunning(const String& animationName) const

  • trunk/Source/WebCore/platform/graphics/ca/PlatformCALayer.h

    r229209 r233268  
    181181    virtual void setSupportsSubpixelAntialiasedText(bool) = 0;
    182182
     183    virtual bool hasContents() const = 0;
    183184    virtual CFTypeRef contents() const = 0;
    184185    virtual void setContents(CFTypeRef) = 0;

  • trunk/Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.h

    r229174 r233268  
    117117    void setSupportsSubpixelAntialiasedText(bool) override;
    118118
     119    bool hasContents() const override;
    119120    CFTypeRef contents() const override;
    120121    void setContents(CFTypeRef) override;

  • trunk/Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.mm

    r232501 r233268  
    392392void PlatformCALayerCocoa::setNeedsDisplay()
    393393{
     394    if (!m_backingStoreAttached)
     395        return;
     396
    394397    BEGIN_BLOCK_OBJC_EXCEPTIONS
    395398    [m_layer setNeedsDisplay];
    … …  
    399402void PlatformCALayerCocoa::setNeedsDisplayInRect(const FloatRect& dirtyRect)
    400403{
     404    if (!m_backingStoreAttached)
     405        return;
     406
    401407    BEGIN_BLOCK_OBJC_EXCEPTIONS
    402408    [m_layer setNeedsDisplayInRect:dirtyRect];
    … …  
    639645    if (attached == m_backingStoreAttached)
    640646        return;
     647
    641648    m_backingStoreAttached = attached;
    642649
    … …  
    738745
    739746    updateContentsFormat();
     747}
     748
     749bool PlatformCALayerCocoa::hasContents() const
     750{
     751    return [m_layer contents];
    740752}
    741753

  • trunk/Source/WebKit/ChangeLog

    r233266 r233268  
     12018-06-27  Simon Fraser  <simon.fraser@apple.com>
     2
     3        https://hackernoon.com/ uses lots of layer backing store
     4        https://bugs.webkit.org/show_bug.cgi?id=186909
     5        rdar://problem/40257540
     6
     7        Reviewed by Tim Horton.
     8       
     9        PlatformCALayerRemote was actually holding onto backing stores for layers with
     10        backing store detached, which could increase memory use. When told that backing stores
     11        are not attached, explicitly throw away the backing, and re-create it (via setNeedsDisplay)
     12        when attached. This is now similar to what PlatformLayerCACocoa does.
     13
     14        * WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.cpp:
     15        (WebKit::PlatformCALayerRemote::setNeedsDisplayInRect):
     16        (WebKit::PlatformCALayerRemote::setNeedsDisplay):
     17        (WebKit::PlatformCALayerRemote::hasContents const):
     18        * WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.h:
     19
    1202018-06-27  Jonathan Bedard  <jbedard@apple.com>
    221

  • trunk/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.cpp

    r229209 r233268  
    191191{
    192192    ASSERT(owner());
    193    
     193
     194    ASSERT(m_properties.backingStoreAttached);
     195
    194196    if (!m_properties.backingStore)
    195197        m_properties.backingStore = std::make_unique<RemoteLayerBackingStore>(this);
    … …  
    203205        return;
    204206
     207    ASSERT(m_properties.backingStoreAttached);
     208
    205209    m_properties.backingStore->ensureBackingStore(m_properties.bounds.size(), m_properties.contentsScale, m_acceleratesDrawing, m_wantsDeepColorBackingStore, m_properties.opaque);
    206210}
    … …  
    208212void PlatformCALayerRemote::setNeedsDisplayInRect(const FloatRect& rect)
    209213{
     214    if (!m_properties.backingStoreAttached)
     215        return;
     216
    210217    ensureBackingStore();
    211218
    … …  
    216223void PlatformCALayerRemote::setNeedsDisplay()
    217224{
     225    if (!m_properties.backingStoreAttached)
     226        return;
     227
    218228    ensureBackingStore();
    219229
    … …  
    537547}
    538548
    539 void PlatformCALayerRemote::setBackingStoreAttached(bool value)
    540 {
    541     if (m_properties.backingStoreAttached == value)
    542         return;
    543 
    544     m_properties.backingStoreAttached = value;
     549void PlatformCALayerRemote::setBackingStoreAttached(bool attached)
     550{
     551    if (m_properties.backingStoreAttached == attached)
     552        return;
     553
     554    m_properties.backingStoreAttached = attached;
    545555    m_properties.notePropertiesChanged(RemoteLayerTreeTransaction::BackingStoreAttachmentChanged);
     556   
     557    if (attached)
     558        setNeedsDisplay();
     559    else
     560        m_properties.backingStore = nullptr;
    546561}
    547562
    … …  
    616631void PlatformCALayerRemote::setSupportsSubpixelAntialiasedText(bool)
    617632{
     633}
     634
     635bool PlatformCALayerRemote::hasContents() const
     636{
     637    return !!m_properties.backingStore;
    618638}
    619639

  • trunk/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.h

    r229174 r233268  
    121121    void setSupportsSubpixelAntialiasedText(bool) override;
    122122
     123    bool hasContents() const override;
    123124    CFTypeRef contents() const override;
    124125    void setContents(CFTypeRef) override;

  • trunk/Source/bmalloc/ChangeLog

    r233216 r233268  
     12018-06-27  Simon Fraser  <simon.fraser@apple.com>
     2
     3        https://hackernoon.com/ uses lots of layer backing store
     4        https://bugs.webkit.org/show_bug.cgi?id=186909
     5        rdar://problem/40257540
     6
     7        Reviewed by Tim Horton.
     8       
     9        Drive-by typo fix.
     10
     11        * bmalloc/Scavenger.cpp:
     12        (bmalloc::dumpStats):
     13
    1142018-06-26  Saam Barati  <sbarati@apple.com>
    215

  • trunk/Source/bmalloc/bmalloc/Scavenger.cpp

    r232599 r233268  
    159159    mach_msg_type_number_t vmSize = TASK_VM_INFO_COUNT;
    160160    if (KERN_SUCCESS == task_info(mach_task_self(), TASK_VM_INFO, (task_info_t)(&vmInfo), &vmSize)) {
    161         dump("phys_footrpint", vmInfo.phys_footprint);
     161        dump("phys_footprint", vmInfo.phys_footprint);
    162162        dump("internal+compressed", vmInfo.internal + vmInfo.compressed);
    163163    }
Note: See TracChangeset for help on using the changeset viewer.