121798 – Consider dropping the render tree while in page cache.

Bug 121798 - Consider dropping the render tree while in page cache.

Summary: Consider dropping the render tree while in page cache.

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	Page Loading (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Andreas Kling

URL:
Keywords:	InRadar

Depends on:	166621
Blocks:
	Show dependency tree / graph

Reported:	2013-09-23 12:41 PDT by Andreas Kling
Modified:	2017-03-03 18:56 PST (History)
CC List:	16 users (show)

See Also:

Attachments
Patch for EWS (5.12 KB, patch) 2013-09-23 14:21 PDT, Andreas Kling	buildbot: commit-queue-	Details \| Formatted Diff \| Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (467.25 KB, application/zip) 2013-09-23 15:06 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (703.67 KB, application/zip) 2013-09-23 17:29 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (750.44 KB, application/zip) 2013-09-23 18:03 PDT, Build Bot	no flags	Details
Patch II for EWS (6.54 KB, patch) 2013-12-05 06:47 PST, Andreas Kling	buildbot: commit-queue-	Details \| Formatted Diff \| Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (453.87 KB, application/zip) 2013-12-05 07:46 PST, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (498.83 KB, application/zip) 2013-12-05 08:19 PST, Build Bot	no flags	Details
Patch III for EWS (5.62 KB, patch) 2016-12-17 07:55 PST, Andreas Kling	buildbot: commit-queue-	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (1.27 MB, application/zip) 2016-12-17 09:03 PST, Build Bot	no flags	Details
Patch (4.01 KB, patch) 2016-12-19 18:37 PST, Andreas Kling	buildbot: commit-queue-	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.05 MB, application/zip) 2016-12-19 19:46 PST, Build Bot	no flags	Details
Patch (3.71 KB, patch) 2016-12-19 22:52 PST, Andreas Kling	buildbot: commit-queue-	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (12.68 MB, application/zip) 2016-12-20 00:12 PST, Build Bot	no flags	Details
Patch (5.19 KB, patch) 2016-12-29 05:34 PST, Andreas Kling	no flags	Details \| Formatted Diff \| Diff
Patch (7.08 KB, patch) 2017-01-02 05:50 PST, Andreas Kling	koivisto: review+	Details \| Formatted Diff \| Diff
Patch for landing (7.27 KB, patch) 2017-01-02 10:02 PST, Andreas Kling	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (15) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Andreas Kling 2013-09-23 12:41:58 PDT

We can make some decent memory savings by losing the render tree when putting pages into the page cache.
Render tree sizes vary depending on content. For scale, the average Wikipedia article uses around ~1.5 MB.

More importantly, this would allow us to tighten things up by not having to worry about the suspended render tree hanging off of the page cache. We currently have numerous short-circuit hacks to avoid badness when Document::inPageCache() returns true. Most of this could go away.

The tradeoff (cost) is that we now have to rebuild the render tree from scratch if/when the user navigates back to a page in page cache. This should already be a very fast operation, and one that we will continue to optimize.

Comment 1 Andreas Kling 2013-09-23 14:21:54 PDT

Created attachment 212391 [details]
Patch for EWS

Comment 2 Build Bot 2013-09-23 15:06:49 PDT

Comment on attachment 212391 [details]
Patch for EWS

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

New failing tests:
compositing/iframes/page-cache-layer-tree.html
fast/loader/scroll-position-restored-on-back.html

Comment 3 Build Bot 2013-09-23 15:06:52 PDT

Created attachment 212396 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

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

Comment 4 Build Bot 2013-09-23 17:29:51 PDT

Comment on attachment 212391 [details]
Patch for EWS

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

New failing tests:
compositing/iframes/page-cache-layer-tree.html
fast/loader/scroll-position-restored-on-back.html

Comment 5 Build Bot 2013-09-23 17:29:53 PDT

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

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

Comment 6 Build Bot 2013-09-23 18:03:37 PDT

Comment on attachment 212391 [details]
Patch for EWS

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

New failing tests:
compositing/iframes/page-cache-layer-tree.html
fast/loader/scroll-position-restored-on-back.html

Comment 7 Build Bot 2013-09-23 18:03:41 PDT

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

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

Comment 8 Andreas Kling 2013-12-05 06:47:34 PST

Created attachment 218511 [details]
Patch II for EWS

Comment 9 Build Bot 2013-12-05 07:46:05 PST

Comment on attachment 218511 [details]
Patch II for EWS

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

New failing tests:
compositing/iframes/page-cache-layer-tree.html

Comment 10 Build Bot 2013-12-05 07:46:07 PST

Created attachment 218514 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

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

Comment 11 Build Bot 2013-12-05 08:19:09 PST

Comment on attachment 218511 [details]
Patch II for EWS

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

New failing tests:
compositing/iframes/page-cache-layer-tree.html

Comment 12 Build Bot 2013-12-05 08:19:11 PST

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

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

Comment 13 Andreas Kling 2016-12-17 07:55:28 PST

Created attachment 297406 [details]
Patch III for EWS

Okay, it's been 3 years, let's try this again.

Comment 14 WebKit Commit Bot 2016-12-17 07:57:04 PST

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


ERROR: Source/WebCore/ChangeLog:10:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 5 files


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

Comment 15 Build Bot 2016-12-17 09:03:24 PST

Comment on attachment 297406 [details]
Patch III for EWS

Attachment 297406 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2744113

New failing tests:
fast/scrolling/iframe-scrollable-after-back.html

Comment 16 Build Bot 2016-12-17 09:03:28 PST

Created attachment 297408 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

Comment 17 Andreas Kling 2016-12-19 18:37:04 PST

Created attachment 297498 [details]
Patch

I think this can actually work.

Comment 18 Build Bot 2016-12-19 19:46:04 PST

Comment on attachment 297498 [details]
Patch

Attachment 297498 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2756933

New failing tests:
fast/scrolling/iframe-scrollable-after-back.html

Comment 19 Build Bot 2016-12-19 19:46:09 PST

Created attachment 297501 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

Comment 20 Andreas Kling 2016-12-19 22:52:57 PST

Created attachment 297511 [details]
Patch

Comment 21 Build Bot 2016-12-20 00:12:35 PST

Comment on attachment 297511 [details]
Patch

Attachment 297511 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2757826

New failing tests:
canvas/philip/tests/2d.canvas.reference.html

Comment 22 Build Bot 2016-12-20 00:12:41 PST

Created attachment 297513 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

Comment 23 Andreas Kling 2016-12-29 05:34:02 PST

Created attachment 297832 [details]
Patch

Comment 24 WebKit Commit Bot 2016-12-30 00:55:16 PST

Comment on attachment 297832 [details]
Patch

Clearing flags on attachment: 297832

Committed r210206: <http://trac.webkit.org/changeset/210206>

Comment 25 WebKit Commit Bot 2016-12-30 00:55:22 PST

All reviewed patches have been landed.  Closing bug.

Comment 26 WebKit Commit Bot 2016-12-30 05:57:27 PST

Re-opened since this is blocked by bug 166621

Comment 27 Andreas Kling 2016-12-30 06:00:38 PST

(In reply to comment #26)
> Re-opened since this is blocked by bug 166621

Rolling out this iteration due to this crash seen on a PLT bot:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x0000000106d05991 WebCore::RenderView::imageQualityController() + 17 (memory:2710)
1   com.apple.WebCore             	0x0000000106b9d06d WebCore::RenderBoxModelObject::willBeDestroyed() + 317 (ImageQualityController.h:50)
2   com.apple.WebCore             	0x0000000106c689b6 WebCore::RenderObject::destroy() + 22 (RenderWidget.h:120)
3   com.apple.WebCore             	0x0000000106c73493 WebCore::RenderScrollbar::setParent(WebCore::ScrollView*) + 83 (HashTable.h:1155)
4   com.apple.WebCore             	0x0000000106d8ae98 WebCore::ScrollView::removeChild(WebCore::Widget&) + 24 (ScrollView.cpp:79)
5   com.apple.WebCore             	0x0000000106d8b019 WebCore::ScrollView::setHasScrollbarInternal(WTF::RefPtr<WebCore::Scrollbar>&, WebCore::ScrollbarOrientation, bool, bool*) + 105 (utility:753)
6   com.apple.WebCore             	0x00000001062a9d8a WebCore::FrameView::detachCustomScrollbars() + 42 (RefPtr.h:64)
7   com.apple.WebCore             	0x0000000106121fdc WebCore::Document::destroyRenderTree() + 172 (Document.cpp:2236)
8   com.apple.WebCore             	0x000000010612d481 WebCore::Document::setPageCacheState(WebCore::Document::PageCacheState) + 65 (Document.cpp:4533)

Looks like a subframe's RenderScrollbar is trying to access its RenderView after said RenderView has been destroyed. Something about destruction order got messed up here.

Meanwhile, this looks like a 2% progression on iOS memory tests, so that part's great :)

Comment 28 Andreas Kling 2017-01-02 05:50:36 PST

Created attachment 297892 [details]
Patch

The render tree needs to be destroyed in depth-first order, or we run into trouble when tearing down pages with subframes. This was caught by bots.

Comment 29 Antti Koivisto 2017-01-02 05:56:24 PST

Comment on attachment 297892 [details]
Patch

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

> Source/WebCore/history/PageCache.cpp:373
> +static void destroyRenderTree(Frame* frame)
> +{
> +    if (!frame)
> +        return;

I would null check in caller.

Comment 30 Antti Koivisto 2017-01-02 05:59:39 PST

Comment on attachment 297892 [details]
Patch

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

> Source/WebCore/history/PageCache.cpp:374
> +static void destroyRenderTree(Frame* frame)
> +{
> +    if (!frame)
> +        return;
> +    destroyRenderTree(frame->tree().traverseNext());

Actually it would be nicer to do this without recursion, with a Vector.

Comment 31 Andreas Kling 2017-01-02 10:02:48 PST

Created attachment 297901 [details]
Patch for landing

Comment 32 WebKit Commit Bot 2017-01-02 13:17:03 PST

Comment on attachment 297901 [details]
Patch for landing

Clearing flags on attachment: 297901

Committed r210226: <http://trac.webkit.org/changeset/210226>

Comment 33 WebKit Commit Bot 2017-01-02 13:17:11 PST

All reviewed patches have been landed.  Closing bug.

Comment 34 Radar WebKit Bug Importer 2017-02-17 17:01:43 PST

<rdar://problem/30588027>

Comment 35 Simon Fraser (smfr) 2017-03-03 18:56:08 PST

This change made this code a lie:

inline bool RenderObject::documentBeingDestroyed() const
{
    return document().renderTreeBeingDestroyed();
}