Bug 225007

Summary: Reduce memory footprint for background tabs
Product: WebKit Reporter: Ben Nham <nham>
Component: New BugsAssignee: Ben Nham <nham>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, ews-watchlist, nham, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
patch for landing
none
Patch none

Description Ben Nham 2021-04-23 16:06:04 PDT
Reduce memory footprint for background tabs
Comment 1 Ben Nham 2021-04-23 16:10:53 PDT
Created attachment 426961 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2021-04-23 16:11:36 PDT
<rdar://problem/77090852>
Comment 3 Ben Nham 2021-04-23 16:12:29 PDT
Comment on attachment 426961 [details]
Patch

initial pass for testing
Comment 4 Chris Dumez 2021-04-23 16:36:37 PDT
Comment on attachment 426961 [details]
Patch

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

> Source/WTF/ChangeLog:9
> +        similar just before WebContent suspends.

The major difference is that on iOS the process suspends and does not keep allocating memory after we simulate memory pressure.

> Source/WTF/wtf/PlatformEnableCocoa.h:393
> +#if !defined(ENABLE_NON_VISIBLE_WEBPROCESS_MEMORY_CLEANUP_TIMER)

Did you mean to enable this only on macOS?

> Source/WebKit/WebProcess/WebProcess.cpp:1590
> +#if ENABLE(NON_VISIBLE_WEBPROCESS_MEMORY_CLEANUP_TIMER)

This should probably cover the whole timer logic. Why start a timer to do nothing on iOS?

> Source/WebKit/WebProcess/WebProcess.cpp:1591
> +    WebCore::releaseMemory(WTF::Critical::Yes, WTF::Synchronous::No, WebCore::MaintainBackForwardCache::No, WebCore::MaintainMemoryCache::No);

One concern. Consider the case where we navigate cross-origin (Process-swap-on-navigation). To support back/forward cache, we keep the previous process around with its back/forward cache entry. When we swapped process, I am assuming that pageWillLeaveWindow() gets called in the previous process. If so, it would start your 1min timer and it would clear my back/forward cache entry. This means that the back/forward cache only works for 1min after a process-swap. If so, this seems bad.
Comment 5 Ben Nham 2021-04-26 13:11:19 PDT
Created attachment 427084 [details]
Patch
Comment 6 Ben Nham 2021-04-26 13:17:08 PDT
Comment on attachment 427084 [details]
Patch

Enable only for Mac and Catalyst and don't drop the back/forward cache.
Comment 7 Chris Dumez 2021-04-26 13:37:13 PDT
Comment on attachment 427084 [details]
Patch

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

r- because this clears the back/forward cache and thus likely breaks the back/forward cache after 1 min, after navigating cross-origin (process-swap).

> Source/WebKit/WebProcess/WebProcess.cpp:1600
> +    WebCore::releaseMemory(WTF::Critical::Yes, WTF::Synchronous::No, WebCore::MaintainBackForwardCache::Yes, WebCore::MaintainMemoryCache::No);

You are still clearing the back/forward cache, which is the one I complained about.

WTF:: is unnecessary, so is WebCore::.

> Source/WebKit/WebProcess/WebProcess.cpp:1602
> +        page->releaseMemory(WTF::Critical::Yes);

WTF:: is unnecessary.
Comment 8 Chris Dumez 2021-04-26 13:39:45 PDT
Comment on attachment 427084 [details]
Patch

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

r=me with nits.

>> Source/WebKit/WebProcess/WebProcess.cpp:1600
>> +    WebCore::releaseMemory(WTF::Critical::Yes, WTF::Synchronous::No, WebCore::MaintainBackForwardCache::Yes, WebCore::MaintainMemoryCache::No);
> 
> You are still clearing the back/forward cache, which is the one I complained about.
> 
> WTF:: is unnecessary, so is WebCore::.

Oh, I misread, this is keeping the back/forward cache. My bad.
Comment 9 Ben Nham 2021-04-26 13:57:02 PDT
Created attachment 427088 [details]
patch for landing
Comment 10 EWS 2021-04-26 16:42:11 PDT
Committed r276619 (237048@main): <https://commits.webkit.org/237048@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427088 [details].
Comment 11 Ben Nham 2021-04-29 09:26:47 PDT
Reverted r276619 for reason:

Causes multiple regressions on perf bots.

Committed r276779 (237162@main): <https://commits.webkit.org/237162@main>
Comment 12 Ben Nham 2021-04-29 09:41:23 PDT
Created attachment 427351 [details]
Patch
Comment 13 Ben Nham 2021-04-29 09:42:21 PDT
*** Bug 225165 has been marked as a duplicate of this bug. ***
Comment 14 EWS 2021-04-29 12:02:02 PDT
Committed r276788 (237166@main): <https://commits.webkit.org/237166@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427351 [details].