Bug 145633 - [WK2] Prune more resources from the MemoryCache before process suspension
Summary: [WK2] Prune more resources from the MemoryCache before process suspension
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-03 22:28 PDT by Chris Dumez
Modified: 2015-06-04 11:24 PDT (History)
4 users (show)

See Also:


Attachments
Patch (17.81 KB, patch)
2015-06-03 22:53 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (18.94 KB, patch)
2015-06-03 22:57 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (21.05 KB, patch)
2015-06-03 23:19 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (21.44 KB, patch)
2015-06-03 23:23 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (21.67 KB, patch)
2015-06-03 23:26 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (21.73 KB, patch)
2015-06-04 10:12 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (21.85 KB, patch)
2015-06-04 10:35 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2015-06-03 22:28:42 PDT
Prune more resources from the MemoryCache before process suspension or on simulated memory warning by doing a synchronous JS garbage collection and sweep *before* pruning dead resources from the memory cache. Previously, we would do the garbage collection after pruning the MemoryCache which meant that a lot of resources became dead after we tried to prune them.

At the end a basic browsing on apple.com, we are able to remove ~17% more resources from the MemoryCache on simulated memory warning with this change.

Pruning as much as we can from the memory cache on critical memory pressure or before process suspension is not only useful to free up memory but also to make room in the vnode table as a lot of CachedResources are mmmap'd from the network disk cache.
Comment 1 Chris Dumez 2015-06-03 22:53:12 PDT
Created attachment 254246 [details]
Patch
Comment 2 Chris Dumez 2015-06-03 22:57:06 PDT
Created attachment 254247 [details]
Patch
Comment 3 Chris Dumez 2015-06-03 23:19:42 PDT
Created attachment 254249 [details]
Patch
Comment 4 Chris Dumez 2015-06-03 23:23:22 PDT
Created attachment 254251 [details]
Patch
Comment 5 Chris Dumez 2015-06-03 23:26:14 PDT
Created attachment 254253 [details]
Patch
Comment 6 Chris Dumez 2015-06-03 23:33:45 PDT
Ah, green bubbles :)
Comment 7 Chris Dumez 2015-06-04 10:12:12 PDT
Created attachment 254267 [details]
Patch
Comment 8 Andreas Kling 2015-06-04 10:28:19 PDT
Comment on attachment 254267 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254267&action=review

r=me this is great.

> Source/WebCore/platform/MemoryPressureHandler.cpp:148
> +    // Do a full sweep of collected objects.

Sidenote (not really about this patch):

If we take the garbageCollectNow() path above, it will do a full sweep before returning.

If we take the garbageCollectSoon() path, it won't generate any new garbage to sweep, since it only accelerates the next collection, but will never actually perform it.
What will happen here is that if we recently did a collection that generated work for the IncrementalSweeper and that work is not yet finished, it will synchronously finish now.
I think it's a good thing to be doing here, just wanted to clarify what's really going on.
Comment 9 Chris Dumez 2015-06-04 10:31:33 PDT
(In reply to comment #8)
> Comment on attachment 254267 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254267&action=review
> 
> r=me this is great.
> 
> > Source/WebCore/platform/MemoryPressureHandler.cpp:148
> > +    // Do a full sweep of collected objects.
> 
> Sidenote (not really about this patch):
> 
> If we take the garbageCollectNow() path above, it will do a full sweep
> before returning.

I wasn't aware, I'll tweak the code accordingly before landing.

 
> If we take the garbageCollectSoon() path, it won't generate any new garbage
> to sweep, since it only accelerates the next collection, but will never
> actually perform it.
> What will happen here is that if we recently did a collection that generated
> work for the IncrementalSweeper and that work is not yet finished, it will
> synchronously finish now.
> I think it's a good thing to be doing here, just wanted to clarify what's
> really going on.

It was my understanding that this still was useful.

Ideally, we would pass a callback to garbageCollectSoon() so that we can do a sweep and prune dead resources from the memory cache once finished.
Comment 10 Chris Dumez 2015-06-04 10:35:18 PDT
Created attachment 254270 [details]
Patch
Comment 11 WebKit Commit Bot 2015-06-04 11:24:09 PDT
Comment on attachment 254270 [details]
Patch

Clearing flags on attachment: 254270

Committed r185206: <http://trac.webkit.org/changeset/185206>
Comment 12 WebKit Commit Bot 2015-06-04 11:24:14 PDT
All reviewed patches have been landed.  Closing bug.