Changeset 195430 in webkit


Ignore:
Timestamp:
Jan 21, 2016 6:00:42 PM (8 years ago)
Author:
akling@apple.com
Message:

CGImageSource sometimes retains temporary SharedBuffer data indefinitely, doubling memory cost.
<https://webkit.org/b/153325>

Reviewed by Anders Carlsson.

After a resource has finished downloading, and has been cached to disk cache,
we mmap() the disk cached version so we can throw out the temporary download buffer.

Due to the way CGImageSource works on Mac/iOS, it's not possible to replace the data
being decoded once the image has been fully decoded once. When doing the replacement,
we'd end up with the SharedBuffer wrapping the mmap() data, and the CGImageSource
keeping the old SharedBuffer::DataBuffer alive, effectively doubling the memory cost.

This patch adds a CachedResource::didReplaceSharedBufferContents() callback that
CachedImage implements to throw out the decoded data. This is currently the only way
to make CGImageSource drop the retain it holds on the SharedBuffer::DataBuffer.
The downside of this approach is that we'll sometimes incur the cost of one additional
image decode after an image downloads and is cached for the first time.

I put a FIXME in there since we could do better with a little help from CGImageSource.

  • loader/cache/CachedImage.cpp:

(WebCore::CachedImage::didReplaceSharedBufferContents):

  • loader/cache/CachedImage.h:
  • loader/cache/CachedResource.cpp:

(WebCore::CachedResource::tryReplaceEncodedData):

  • loader/cache/CachedResource.h:

(WebCore::CachedResource::didReplaceSharedBufferContents):

Location:
trunk/Source/WebCore
Files:
5 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebCore/ChangeLog

    r195421 r195430  
     12016-01-21  Andreas Kling  <akling@apple.com>
     2
     3        CGImageSource sometimes retains temporary SharedBuffer data indefinitely, doubling memory cost.
     4        <https://webkit.org/b/153325>
     5
     6        Reviewed by Anders Carlsson.
     7
     8        After a resource has finished downloading, and has been cached to disk cache,
     9        we mmap() the disk cached version so we can throw out the temporary download buffer.
     10
     11        Due to the way CGImageSource works on Mac/iOS, it's not possible to replace the data
     12        being decoded once the image has been fully decoded once. When doing the replacement,
     13        we'd end up with the SharedBuffer wrapping the mmap() data, and the CGImageSource
     14        keeping the old SharedBuffer::DataBuffer alive, effectively doubling the memory cost.
     15
     16        This patch adds a CachedResource::didReplaceSharedBufferContents() callback that
     17        CachedImage implements to throw out the decoded data. This is currently the only way
     18        to make CGImageSource drop the retain it holds on the SharedBuffer::DataBuffer.
     19        The downside of this approach is that we'll sometimes incur the cost of one additional
     20        image decode after an image downloads and is cached for the first time.
     21
     22        I put a FIXME in there since we could do better with a little help from CGImageSource.
     23
     24        * loader/cache/CachedImage.cpp:
     25        (WebCore::CachedImage::didReplaceSharedBufferContents):
     26        * loader/cache/CachedImage.h:
     27        * loader/cache/CachedResource.cpp:
     28        (WebCore::CachedResource::tryReplaceEncodedData):
     29        * loader/cache/CachedResource.h:
     30        (WebCore::CachedResource::didReplaceSharedBufferContents):
     31
    1322016-01-21  Beth Dakin  <bdakin@apple.com>
    233
  • trunk/Source/WebCore/loader/cache/CachedImage.cpp

    r191369 r195430  
    437437}
    438438
     439void CachedImage::didReplaceSharedBufferContents()
     440{
     441    if (m_image) {
     442        // Let the Image know that the SharedBuffer has been rejigged, so it can let go of any references to the heap-allocated resource buffer.
     443        // FIXME(rdar://problem/24275617): It would be better if we could somehow tell the Image's decoder to swap in the new contents without destroying anything.
     444        m_image->destroyDecodedData(true);
     445    }
     446    CachedResource::didReplaceSharedBufferContents();
     447}
     448
    439449void CachedImage::error(CachedResource::Status status)
    440450{
  • trunk/Source/WebCore/loader/cache/CachedImage.h

    r184315 r195430  
    128128    void addIncrementalDataBuffer(SharedBuffer&);
    129129
     130    void didReplaceSharedBufferContents() override;
     131
    130132    typedef std::pair<LayoutSize, float> SizeAndZoom;
    131133    typedef HashMap<const CachedImageClient*, SizeAndZoom> ContainerSizeRequests;
  • trunk/Source/WebCore/loader/cache/CachedResource.cpp

    r195406 r195430  
    808808        return;
    809809
    810     if (m_data->tryReplaceContentsWithPlatformBuffer(newBuffer)) {
    811         // FIXME: Should we call checkNotify() here to move already-decoded images to the new data source?
    812     }
    813 }
    814 
    815 #endif
    816 
    817 }
     810    if (m_data->tryReplaceContentsWithPlatformBuffer(newBuffer))
     811        didReplaceSharedBufferContents();
     812}
     813
     814#endif
     815
     816}
  • trunk/Source/WebCore/loader/cache/CachedResource.h

    r194529 r195430  
    268268    void didAccessDecodedData(double timeStamp);
    269269
     270    virtual void didReplaceSharedBufferContents() { }
     271
    270272    // FIXME: Make the rest of these data members private and use functions in derived classes instead.
    271273    HashCountedSet<CachedResourceClient*> m_clients;
Note: See TracChangeset for help on using the changeset viewer.