RESOLVED FIXED 135939
Remove PurgeableBuffer since it is not very useful any more
https://bugs.webkit.org/show_bug.cgi?id=135939
Summary Remove PurgeableBuffer since it is not very useful any more
Pratik Solanki
Reported 2014-08-14 07:58:31 PDT
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.
Attachments
Patch for bots (51.60 KB, patch)
2014-08-14 07:59 PDT, Pratik Solanki
no flags
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (346.79 KB, application/zip)
2014-08-14 10:01 PDT, Build Bot
no flags
Patch (71.30 KB, patch)
2014-08-14 11:52 PDT, Pratik Solanki
no flags
Patch (71.55 KB, patch)
2014-08-19 15:31 PDT, Pratik Solanki
kling: review+
Pratik Solanki
Comment 1 2014-08-14 07:59:22 PDT
Created attachment 236589 [details] Patch for bots
WebKit Commit Bot
Comment 2 2014-08-14 08:03:54 PDT
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 3 2014-08-14 10:01:51 PDT
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
Comment 4 2014-08-14 10:01:56 PDT
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
Comment 5 2014-08-14 11:52:54 PDT
Geoffrey Garen
Comment 6 2014-08-14 11:54:15 PDT
Comment on attachment 236607 [details] Patch r=me
Pratik Solanki
Comment 7 2014-08-18 16:26:12 PDT
WebKit Commit Bot
Comment 8 2014-08-18 22:06:33 PDT
Re-opened since this is blocked by bug 136060
Ryosuke Niwa
Comment 9 2014-08-18 22:12:07 PDT
This patch caused 14% PLT3 regression on Yosemite. See the email I sent internally.
Pratik Solanki
Comment 10 2014-08-18 22:14:11 PDT
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
Comment 11 2014-08-19 15:29:44 PDT
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
Comment 12 2014-08-19 15:31:19 PDT
Pratik Solanki
Comment 13 2014-08-19 15:31:51 PDT
(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 14 2014-08-19 15:47:36 PDT
Comment on attachment 236828 [details] Patch r=me
Pratik Solanki
Comment 15 2014-08-19 16:56:02 PDT
Note You need to log in before you can comment on or make changes to this bug.