Bug 62675 - Minimize memory due to layer backing stores for pages in the Page Cache
Summary: Minimize memory due to layer backing stores for pages in the Page Cache
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-06-14 16:10 PDT by Joseph Pecoraro
Modified: 2011-06-20 17:02 PDT (History)
7 users (show)

See Also:


Attachments
[TEST] Test Page for numerous layers and iframes with layers. (2.75 KB, application/zip)
2011-06-14 16:21 PDT, Joseph Pecoraro
no flags Details
[PATCH] Clear Backing Stores entering Page Cache (8.32 KB, patch)
2011-06-14 16:23 PDT, Joseph Pecoraro
simon.fraser: review+
Details | Formatted Diff | Diff
[PATCH] LayoutTest for this (8.16 KB, patch)
2011-06-20 16:17 PDT, Joseph Pecoraro
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2011-06-14 16:10:46 PDT
Pages in the PageCache preserve their layers and backing stores and
can consume a lot of memory. In situations where we need to handle
low memory conditions we can still keep the Document in memory if
we save enough memory by just clearing the backing stores + layers.
This can also be done proactively when a page enters the PageCache.
Comment 1 Joseph Pecoraro 2011-06-14 16:21:46 PDT
Created attachment 97183 [details]
[TEST] Test Page for numerous layers and iframes with layers.

Test case I used. I saw ~90MB of CoreAnimation memory with Desktop Safari.
With my patch, and the feature enabled, that memory dropped to 4KB when
going to about:blank, and restored to 90 when going back to the page.
Comment 2 Joseph Pecoraro 2011-06-14 16:23:51 PDT
Created attachment 97185 [details]
[PATCH] Clear Backing Stores entering Page Cache

First attempt.
Comment 3 WebKit Review Bot 2011-06-14 16:26:18 PDT
Attachment 97185 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/RenderLayer.cpp:3648:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Joseph Pecoraro 2011-06-14 16:34:18 PDT
<rdar://problem/8652717>
Comment 5 Simon Fraser (smfr) 2011-06-17 11:48:46 PDT
Comment on attachment 97185 [details]
[PATCH] Clear Backing Stores entering Page Cache

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

> Source/WebCore/ChangeLog:13
> +        This only affects memory usage and is disabled by default, so no test.

It would be nice to have a test that puts a composited page in the BF cache, brings it back, then calls layerTreeAsText(). That way at least platforms that enable the option will have test coverage.
Comment 6 Simon Fraser (smfr) 2011-06-17 11:51:48 PDT
Actually, it would be good to test that this doesn't break WebGL and <video> if enabled on desktop.
Comment 7 Eric Seidel (no email) 2011-06-18 13:43:25 PDT
Attachment 97185 [details] was posted by a committer and has review+, assigning to Joseph Pecoraro for commit.
Comment 8 Joseph Pecoraro 2011-06-20 13:00:32 PDT
(In reply to comment #6)
> Actually, it would be good to test that this doesn't break WebGL and <video> if enabled on desktop.

Verified manually that this didn't break WebGL, <video>, and <audio> when enabled on Desktop.
Comment 9 Joseph Pecoraro 2011-06-20 16:17:09 PDT
Created attachment 97883 [details]
[PATCH] LayoutTest for this

This is a LayoutTest that dumps the layer tree after restoring
a page with composited iframes from the page cache. Testing
this with pageCache()->shouldClearBackingStores() enabled
had the exact same results, which is what we want.
Comment 10 Darin Adler 2011-06-20 16:18:34 PDT
Comment on attachment 97883 [details]
[PATCH] LayoutTest for this

Test looks OK. Will this work on all platforms? If not, then doesn’t it need platform-specific results or skip list items?
Comment 11 Joseph Pecoraro 2011-06-20 16:22:11 PDT
(In reply to comment #10)
> (From update of attachment 97883 [details])
> Test looks OK. Will this work on all platforms? If not, then doesn’t it need
> platform-specific results or skip list items?

It probably does. I wasn't sure so I was going to let the EWS tell me.
Comment 12 Joseph Pecoraro 2011-06-20 16:58:24 PDT
Looked good for EWS ports. Also, LayoutTests/composited/* has a lot of
cross-platform expected results for layer tree dumps. I'll land with the
test in its current spot and watch the build-bots in case I need to make a fix.
Comment 13 Joseph Pecoraro 2011-06-20 17:02:04 PDT
Landed the patch + test together in: r89316
http://trac.webkit.org/changeset/89316