WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
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
Details
Patch
(71.30 KB, patch)
2014-08-14 11:52 PDT
,
Pratik Solanki
no flags
Details
Formatted Diff
Diff
Patch
(71.55 KB, patch)
2014-08-19 15:31 PDT
,
Pratik Solanki
kling
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 236607
[details]
Patch
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
Committed
r172736
: <
http://trac.webkit.org/changeset/172736
>
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
Created
attachment 236828
[details]
Patch
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
Committed
r172790
: <
http://trac.webkit.org/changeset/172790
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug