Bug 135939 – Remove PurgeableBuffer since it is not very useful any more

• Component: Platform
• Status: RESOLVED FIXED

[Pratik Solanki]

The utility of purgeable memory for cached resources has diminished over time. Currently, WebKit uses file backed resources when possible. Since this is read only memory, it is in essence "purgeable". Having the PurgeableBuffer code adds additional complexity that we don't need. e.g. on my Mac, I am not seeing any entry for "WebCore purgeable data" when I run mmap against the web processes that I have running. It is used on iOS, however even there much of the purgeable memory we create gets replaced by file backed memory once we get the notification from CFNetwork.

[Pratik Solanki]

Created attachment 236589 [details]
Patch for bots

[WebKit Commit Bot]

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


ERROR: Source/WebCore/loader/cache/MemoryCache.h:90:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/loader/cache/MemoryCache.h:92:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 6 files


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

[Build Bot]

Comment on attachment 236589 [details]
Patch for bots

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

New failing tests:
editing/pasteboard/drag-selected-image-to-contenteditable.html
css3/masking/mask-repeat-space-padding.html
http/tests/local/link-stylesheet-load-order-preload.html
http/tests/cache/subresource-expiration-2.html
css3/viewport-percentage-lengths/viewport-percentage-lengths-anonymous-block.html
editing/selection/drag-to-contenteditable-iframe.html
css3/viewport-percentage-lengths/viewport-percentage-lengths-percent-size-child.html
http/tests/loading/redirect-methods.html
http/tests/cache/post-redirect-get.php
css3/background/background-repeat-space-content.html
http/tests/cache/history-only-cached-subresource-loads.html
fast/dom/StyleSheet/detached-style.html
http/tests/cache/display-image-unset-allows-cached-image-load.html
http/tests/cache/iframe-304-crash.html
fast/filter-image/filter-image-animation.html
fast/dom/StyleSheet/detached-style-pi-2.xhtml
fast/dom/StyleSheet/detached-style-2.html
fast/files/xhr-response-blob.html
http/tests/cache/post-with-cached-subresources.php
http/tests/cache/history-only-cached-subresource-loads-max-age-https.html
http/tests/cache/subresource-fragment-identifier.html
http/tests/cache/subresource-multiple-instances.html
http/tests/cache/subresource-expiration-1.html
editing/pasteboard/4989774.html
editing/pasteboard/4947130.html
fast/canvas/canvas-scale-drawImage-shadow.html
http/tests/cache/cached-main-resource.html
fast/css/font-face-download-error.html
editing/pasteboard/4641033.html
fast/dom/StyleSheet/detached-style-pi.xhtml

[Build Bot]

Created attachment 236598 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

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

[Pratik Solanki]

Created attachment 236607 [details]
Patch

[Geoffrey Garen]

Comment on attachment 236607 [details]
Patch

r=me

[Pratik Solanki]

Committed r172736: <http://trac.webkit.org/changeset/172736>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 136060

[Ryosuke Niwa]

This patch caused 14% PLT3 regression on Yosemite. See the email I sent internally.

[Pratik Solanki]

14% PLT regression by removing this code. That is awesome! (And frankly a bit unbelievable. I'll take a look at it tomorrow)

[Pratik Solanki]

Problem with the patch was that I removed the call to adjustSize() in MemoryCache::evict()

-        // If the resource was purged, it means we had already decremented the size when we made the
-        // resource purgeable in makeResourcePurgeable(). So adjust the size if we are evicting a
-        // resource that was not marked as purgeable.
-        if (!MemoryCache::shouldMakeResourcePurgeableOnEviction() || !resource->isPurgeable())
-            adjustSize(resource->hasClients(), -static_cast<int>(resource->size()));

We need to keep the call to adjustSize().

[Pratik Solanki]

Created attachment 236828 [details]
Patch

[Pratik Solanki]

(In reply to comment #11)
> Problem with the patch was that I removed the call to adjustSize() in MemoryCache::evict()
> 
> -        // If the resource was purged, it means we had already decremented the size when we made the
> -        // resource purgeable in makeResourcePurgeable(). So adjust the size if we are evicting a
> -        // resource that was not marked as purgeable.
> -        if (!MemoryCache::shouldMakeResourcePurgeableOnEviction() || !resource->isPurgeable())
> -            adjustSize(resource->hasClients(), -static_cast<int>(resource->size()));
> 
> We need to keep the call to adjustSize().

Props to Mr. Kling for spotting this.

[Andreas Kling]

Comment on attachment 236828 [details]
Patch

r=me

[Pratik Solanki]

Committed r172790: <http://trac.webkit.org/changeset/172790>